Bugzilla – Bug 2312
code review: IPv6 support for LTE
Last modified: 2018-02-07 18:53:19 EST
Posted here: https://codereview.appspot.com/234000043
Updated by Manoj's work this past summer: https://github.com/manoj24rana/LTE-IPv6-Review/commit/1c739c0fc1442e7eb0e0e03dbd2691cb5d9851ff
Created attachment 2979 [details] patch against ns-3-dev rev. 13202 List of FAILed tests: lte-cqa-ff-mac-scheduler lte-fdtbfq-ff-mac-scheduler lte-pss-ff-mac-scheduler lte-tdtbfq-ff-mac-scheduler lte-test-deactivate-bearer
Created attachment 2980 [details] new binary dia figure file: epc-data-flow-ul.dia
Created attachment 2981 [details] new binary dia figure file: epc-data-flow-dl.dia
I am not sure what is happening here. I logged EpcSgwPgwApplication, and for ns-3-dev code I am seeing many NS_LOG warnings:"no matching bearer for this packet", and for when I apply patch I see: "unknown UE address ff02::1 unknown UE address ff02::1 " This is if I run test: lte-test-deactivate-bearer, e.g.: NS_LOG="EpcSgwPgwApplication=level_warn" ./waf --run "test-runner --suite=lte-test-deactivate-bearer --verbose". Both of these messages come from function EpcSgwPgwApplication::RecvFromTunDevice(). I am not sure what is the correct behaviour here, I will need to take a further look into this.
ff02::1 is the all-node address. Any node will have it, but (of course) it must not be removed upon a bearer deactivation. I'll try to have a look at it too. T. (In reply to Biljana Bojović from comment #5) > I am not sure what is happening here. I logged EpcSgwPgwApplication, and for > ns-3-dev code I am seeing many NS_LOG warnings:"no matching bearer for this > packet", > and for when I apply patch I see: > > "unknown UE address ff02::1 > unknown UE address ff02::1 > " > > This is if I run test: lte-test-deactivate-bearer, e.g.: > NS_LOG="EpcSgwPgwApplication=level_warn" ./waf --run "test-runner > --suite=lte-test-deactivate-bearer --verbose". > > Both of these messages come from function > EpcSgwPgwApplication::RecvFromTunDevice(). > > I am not sure what is the correct behaviour here, I will need to take a > further look into this.
The issue is the wrongly set port for uplink applications. This was not generating issue before Manoj's patch because tests are only taking into account performance between UE and ENB. However, since Manoj's patch allows ICMP packets to be passed through SGW/PGW, which was not previously the case, now in the case of destination unreachable, the ICMP will try to notify the UE about this. This generates additional downlink traffic betwen eNB and UE so tests fail. I will try to provide patch for this asap.
Patch for tests pushed to changeset: 13229: d2fece511b72. Regarding the code, it seems good to me. I do not understand fully the EPC part of changes, but in general it is very good code. I have just two very minor comments: some names of variables are hard to read, e.g. m_rxLteenbPktTrace -> m_rxLteEnbPktTrace, m_uePgwbaseipv4prefix8 -> m_uePgwBaseIpv4Prefix8 , m_uePgwbaseipv6prefix32 -> m_uePgwBaseIpv6Prefix32, etc. And on couple of places there is a code like this: if (ipType == 0x04) { ... } else { } I would prefer the following (but if you leave as it is now it's ok for me): if (ipType == 0x04) { ... } else if (ipType == 0x06) { ... } else { NS_ASSERT ("unknown ip type..."); }
Hi, I'll try to clean up a bit this patch tonight, then we can push it tomorrow or when you want. I just want to remove the part covered here: https://www.nsnam.org/bugzilla/show_bug.cgi?id=2835 and hold that back until we know what to do with multiple EPCs for what concerns the carrier initialization. T.
Created attachment 3016 [details] Same as previous but w/o the customizable IP addresses in EPC I removed the capability to change the EPC IPv4 and IPv6 addresses because the whole LTE module is not yet multi-EPC safe, and I am ready to bet that someone WILL try to use the attributes to create more than one EPC, with horrible results. Other than that, I think that this can be pushed.
Created attachment 3022 [details] cleaned up patch This is the latest version, with cleaned up code.
Just one more comment. In EpcSgwPgwApplication there are attributes RxfromTun, and RxfromS1u. I would change these to RxFromTun and RxFromS1u, respectively, and since they are used in lte-test-ipv6-routing.cc, the lines that should be updated accordingly are 387 and 388.
Created attachment 3023 [details] re-cleaned patch Done. And I also cleaned up a bit the test in the meantime.
(In reply to Tommaso Pecorella from comment #13) > Created attachment 3023 [details] > re-cleaned patch > > Done. > And I also cleaned up a bit the test in the meantime. Please rename: + void SetLTESocket6(Ptr<Socket> lteSocket6); + Ptr<Socket> GetLTESocket6(); to 'SetLteSocket6' and 'GetLteSocket6'. What is the meaning of this comment (should something else be fixed)? + // we use a reverse iterator since filter priority is not implemented properly. Perhaps consider to remove this bug from the 'Bugs fixed' in RELEASE_NOTES because it is covered already in new features and in New API in CHANGES.html. There is dead code to remove in lte-test-ipv6-routing.cc. Should any examples be hooked to the test system (examples_to_run.py) or are they sufficiently covered by the new test suite? Please also commit the modified dia files.
(In reply to Tom Henderson from comment #14) > (In reply to Tommaso Pecorella from comment #13) > > Created attachment 3023 [details] > > re-cleaned patch > > > > Done. > > And I also cleaned up a bit the test in the meantime. > > Please rename: > + void SetLTESocket6(Ptr<Socket> lteSocket6); > + Ptr<Socket> GetLTESocket6(); > to 'SetLteSocket6' and 'GetLteSocket6'. I removed these 2 functions and I changed the constructor. It is *way* more foolproof than to remember to call SetLteSocket6(). > What is the meaning of this comment (should something else be fixed)? > + // we use a reverse iterator since filter priority is not implemented > properly. No idea at all. It was in the old code and I kept it. > Perhaps consider to remove this bug from the 'Bugs fixed' in RELEASE_NOTES > because it is covered already in new features and in New API in CHANGES.html. Done. > There is dead code to remove in lte-test-ipv6-routing.cc. Removed. Thanks for spotting it. > Should any examples be hooked to the test system (examples_to_run.py) or are > they sufficiently covered by the new test suite? Yes, I forgot to add the 3 examples, and yes, I also forgot to update them to the latest patch. I added and fixed them. > Please also commit the modified dia files. Of course.
I found a last-minute problem involving bug #1745. If I don't fix that one, this patch can't be submitted. I'll work on it tomorrow.
pushed in changeset: 13278:f7d839cee8e9