Bug 2312 - code review: IPv6 support for LTE
code review: IPv6 support for LTE
Status: RESOLVED FIXED
Product: ns-3
Classification: Unclassified
Component: lte
unspecified
PC Linux
: P5 enhancement
Assigned To: Biljana Bojović
:
Depends on: 1745 2768
Blocks:
  Show dependency treegraph
 
Reported: 2016-02-26 18:23 EST by Tom Henderson
Modified: 2018-02-07 18:53 EST (History)
5 users (show)

See Also:


Attachments
patch against ns-3-dev rev. 13202 (100.36 KB, patch)
2017-12-12 16:06 EST, Tom Henderson
Details | Diff
new binary dia figure file: epc-data-flow-ul.dia (4.03 KB, application/octet-stream)
2017-12-12 16:07 EST, Tom Henderson
Details
new binary dia figure file: epc-data-flow-dl.dia (4.00 KB, application/octet-stream)
2017-12-12 16:07 EST, Tom Henderson
Details
Same as previous but w/o the customizable IP addresses in EPC (93.74 KB, patch)
2018-01-25 22:00 EST, Tommaso Pecorella
Details | Diff
cleaned up patch (102.13 KB, patch)
2018-01-30 10:05 EST, Tommaso Pecorella
Details | Diff
re-cleaned patch (102.05 KB, patch)
2018-01-30 16:18 EST, Tommaso Pecorella
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tom Henderson 2016-02-26 18:23:11 EST
Posted here:  https://codereview.appspot.com/234000043
Comment 1 Tom Henderson 2017-12-10 00:12:22 EST
Updated by Manoj's work this past summer:  https://github.com/manoj24rana/LTE-IPv6-Review/commit/1c739c0fc1442e7eb0e0e03dbd2691cb5d9851ff
Comment 2 Tom Henderson 2017-12-12 16:06:24 EST
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
Comment 3 Tom Henderson 2017-12-12 16:07:19 EST
Created attachment 2980 [details]
new binary dia figure file:  epc-data-flow-ul.dia
Comment 4 Tom Henderson 2017-12-12 16:07:51 EST
Created attachment 2981 [details]
new binary dia figure file:  epc-data-flow-dl.dia
Comment 5 Biljana Bojović 2017-12-20 14:12:33 EST
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.
Comment 6 Tommaso Pecorella 2017-12-21 10:11:51 EST
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.
Comment 7 Biljana Bojović 2017-12-21 12:59:26 EST
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.
Comment 8 Biljana Bojović 2017-12-21 19:36:28 EST
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...");
}
Comment 9 Tommaso Pecorella 2018-01-25 18:17:00 EST
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.
Comment 10 Tommaso Pecorella 2018-01-25 22:00:59 EST
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.
Comment 11 Tommaso Pecorella 2018-01-30 10:05:10 EST
Created attachment 3022 [details]
cleaned up patch

This is the latest version, with cleaned up code.
Comment 12 Biljana Bojović 2018-01-30 13:00:04 EST
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.
Comment 13 Tommaso Pecorella 2018-01-30 16:18:13 EST
Created attachment 3023 [details]
re-cleaned patch

Done.
And I also cleaned up a bit the test in the meantime.
Comment 14 Tom Henderson 2018-02-01 18:43:57 EST
(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.
Comment 15 Tommaso Pecorella 2018-02-01 22:07:49 EST
(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.
Comment 16 Tommaso Pecorella 2018-02-05 23:19:10 EST
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.
Comment 17 Tommaso Pecorella 2018-02-07 18:53:19 EST
pushed in changeset:   13278:f7d839cee8e9