GSoC 2012 NS-3 LTE MAC Scheduler Project

GSoC 2012 NS-3 LTE MAC Scheduler Project

[Tom Henderson, 2012-10-29 05:33:47]

I scanned through this code (it is a lot of code) again tonight.  

This is a nice outcome for a GSOC project, with tests, documentation, and lots
of code.  Well done!

My only concern is that there is a lot of API for which the implementation is
marked "// TODO: Implementation of the API", and the function returns while only
printing a log statement.  If it is possible that a future user may call these
methods and their simulations produce incorrect data as a result, then I would
rather see you use "NS_FATAL_ERROR("Unimplemented")" rather than silently
logging the method and proceeding.    Or else, remove the method from the API. 
Can you please review all of these unimplemented methods and modify as
appropriate?

My other inline comments were minor. I suppressed making the same comment across
all files, although most implementation files had similar issues.

http://codereview.appspot.com/6591047/diff/1/src/lte/doc/source/lte-design.rst
File src/lte/doc/source/lte-design.rst (right):

http://codereview.appspot.com/6591047/diff/1/src/lte/doc/source/lte-design.rs...
src/lte/doc/source/lte-design.rst:659: Currently, MT scheduler in LENA has two
versions: frequency domain (FDMT) and time domain (TDMT).
ns-3 instead of LENA?

http://codereview.appspot.com/6591047/diff/1/src/lte/doc/source/lte-design.rs...
src/lte/doc/source/lte-design.rst:734: Token Band Fair Queue (TBFQ) is a QoS
aware scheduler which derives from the leaky-bucket mechanism. In TBFQ,
Token Bank

http://codereview.appspot.com/6591047/diff/1/src/lte/doc/source/lte-reference...
File src/lte/doc/source/lte-references.rst (right):

http://codereview.appspot.com/6591047/diff/1/src/lte/doc/source/lte-reference...
src/lte/doc/source/lte-references.rst:88: .. [FCapo2012] F.Capozzi, G.Piro
L.A.Grieco, G.Boggia, P.Camarda, "Downlink Packet Scheduling in LTE Cellular
Networks: Key Design Issues and a Survey", IEEE Comm. Surveys and Tutorials, to
appear
this seems to be now published in issue 99

http://codereview.appspot.com/6591047/diff/1/src/lte/model/fdbet-ff-mac-sched...
File src/lte/model/fdbet-ff-mac-scheduler.cc (right):

http://codereview.appspot.com/6591047/diff/1/src/lte/model/fdbet-ff-mac-sched...
src/lte/model/fdbet-ff-mac-scheduler.cc:25: #endif /* __FreeBSD__ */
This probably needs to be moved to proposed math.h; see bug 1467

http://codereview.appspot.com/6591047/diff/1/src/lte/model/fdbet-ff-mac-sched...
src/lte/model/fdbet-ff-mac-scheduler.cc:351: // TODO: Implementation of the API
Consider instead to make this, and all such "TODOs", throw an NS_FATAL_ERROR
("Unimplemented").

http://codereview.appspot.com/6591047/diff/1/src/lte/model/fdbet-ff-mac-sched...
src/lte/model/fdbet-ff-mac-scheduler.cc:1005: //NS_LOG_DEBUG (this << " Does not
find info on allocation, size : " << m_allocationMaps.size ());
delete commented out log statements, or uncomment

http://codereview.appspot.com/6591047/diff/1/src/lte/model/fdbet-ff-mac-sched...
File src/lte/model/fdbet-ff-mac-scheduler.h (right):

http://codereview.appspot.com/6591047/diff/1/src/lte/model/fdbet-ff-mac-sched...
src/lte/model/fdbet-ff-mac-scheduler.h:90: // (See 4.1 for description of the
primitives)
4.1 of what?

[Dizhi.Zhou, 2012-11-30 22:43:06]

Hi Tom,

I update GSoC codes based on your comment. Please find detail below. The new
version has already push to repository:
http://code.nsnam.org/dizhizhou/ns-3-dev

Best regards
Dizhi

https://codereview.appspot.com/6591047/diff/1/src/lte/doc/source/lte-design.rst
File src/lte/doc/source/lte-design.rst (right):

https://codereview.appspot.com/6591047/diff/1/src/lte/doc/source/lte-design.r...
src/lte/doc/source/lte-design.rst:659: Currently, MT scheduler in LENA has two
versions: frequency domain (FDMT) and time domain (TDMT).
On 2012/10/29 05:33:47, Tom Henderson wrote:
> ns-3 instead of LENA?

Done.

https://codereview.appspot.com/6591047/diff/1/src/lte/doc/source/lte-design.r...
src/lte/doc/source/lte-design.rst:734: Token Band Fair Queue (TBFQ) is a QoS
aware scheduler which derives from the leaky-bucket mechanism. In TBFQ,
On 2012/10/29 05:33:47, Tom Henderson wrote:
> Token Bank

Done.

https://codereview.appspot.com/6591047/diff/1/src/lte/doc/source/lte-referenc...
File src/lte/doc/source/lte-references.rst (right):

https://codereview.appspot.com/6591047/diff/1/src/lte/doc/source/lte-referenc...
src/lte/doc/source/lte-references.rst:88: .. [FCapo2012] F.Capozzi, G.Piro
L.A.Grieco, G.Boggia, P.Camarda, "Downlink Packet Scheduling in LTE Cellular
Networks: Key Design Issues and a Survey", IEEE Comm. Surveys and Tutorials, to
appear
On 2012/10/29 05:33:47, Tom Henderson wrote:
> this seems to be now published in issue 99

Done.

https://codereview.appspot.com/6591047/diff/1/src/lte/model/fdbet-ff-mac-sche...
File src/lte/model/fdbet-ff-mac-scheduler.cc (right):

https://codereview.appspot.com/6591047/diff/1/src/lte/model/fdbet-ff-mac-sche...
src/lte/model/fdbet-ff-mac-scheduler.cc:25: #endif /* __FreeBSD__ */
I updated GSoC code to NS-3.15 in Oct.21. These code were removed in the latest
version.

On 2012/10/29 05:33:47, Tom Henderson wrote:
> This probably needs to be moved to proposed math.h; see bug 1467

https://codereview.appspot.com/6591047/diff/1/src/lte/model/fdbet-ff-mac-sche...
src/lte/model/fdbet-ff-mac-scheduler.cc:351: // TODO: Implementation of the API
Change to following codes:
NS_FATAL_ERROR ("unimplemented"):
return;

Hi Tome, do you think we also need to show the API's name in NS_FATAL_ERROR? In
this way, user can locate which API they incorrectly used.
On 2012/10/29 05:33:47, Tom Henderson wrote:
> Consider instead to make this, and all such "TODOs", throw an NS_FATAL_ERROR
> ("Unimplemented").

https://codereview.appspot.com/6591047/diff/1/src/lte/model/fdbet-ff-mac-sche...
src/lte/model/fdbet-ff-mac-scheduler.cc:1005: //NS_LOG_DEBUG (this << " Does not
find info on allocation, size : " << m_allocationMaps.size ());
On 2012/10/29 05:33:47, Tom Henderson wrote:
> delete commented out log statements, or uncomment

Done.

https://codereview.appspot.com/6591047/diff/1/src/lte/model/fdbet-ff-mac-sche...
File src/lte/model/fdbet-ff-mac-scheduler.h (right):

https://codereview.appspot.com/6591047/diff/1/src/lte/model/fdbet-ff-mac-sche...
src/lte/model/fdbet-ff-mac-scheduler.h:90: // (See 4.1 for description of the
primitives)
This is the "LTE MAC Scheduler Interface Specification 1.1". It is explained in
In LTE manual.
On 2012/10/29 05:33:47, Tom Henderson wrote:
> 4.1 of what?