Bug 3010

Created attachment 3208 [details]
proposed patch

Everything is working fine and tested, but I still have one regression issue.

This test is failing:
src/traffic-control/examples/adaptive-red-tests --testNumber=13

This is due to the fact that I increase the default Queue::MaxSize from 100p to 300p.
The reason I did that is to avoid that users do have to configure many parameters to have extended A-MPDU working.

However, I have not really managed to solve this problem in traffic-control, somebody could help on that?

(In reply to sebastien.deronne from comment #1)
> Created attachment 3208 [details]
> proposed patch

Sebastien, I reviewed the patch and have questions about usage and defaults.

First, you modify default Queue size, which probably contributes to the regression failure in traffic-control module.  Can you find a solution that does not modify the network module?

Second, it seems that this feature is not default when 11ax is configured; should it be?

It also seems that two steps are needed to enable this feature.

1) change attribute HeConfiguration::MpduBufferSize from 64 to 256
2) change MaxAmpduSize in the MpduAggregator.

Regarding 2), the test code performs a low-level configuration that I would not expect users to have to do:

  m_mac->GetBEQueue ()->GetMpduAggregator ()->SetMaxAmpduSize (4194304); //Maximum size allowed by 802.11ax

However, the higher-level approach to this seems to be to change four RegularWifiMac attributes from 65535 to 4194304; each of:

BE_MaxAmpduSize
BK_MaxAmpduSize
VO_MaxAmpduSize
VI_MaxAmpduSize


In summary, I would prefer to either make the setting of the above 5 attributes default in 11ax if this is the appropriate default, and if not, I would suggest to create a helper method that sets these five attributes.

  WifiHeHelper wifiHeHelper;
  wifiHeHelper.EnableMaxAmpduAggregation ();
  ...

(we haven't talked yet about a WifiHeHelper so the above API is provisional)

Longer term, it would also help if there were some kind of example program using the preferred high-level API that contrasts and documents the maximum throughput available for 11n, 11ac, and 11ax (by showing by example how to tune these configurations to maximize throughput, and by stating what the ns-3 implementation of these standards is able to maximally achieve).

(In reply to Tom Henderson from comment #3)
> (In reply to sebastien.deronne from comment #1)
> > Created attachment 3208 [details]
> > proposed patch
> 
> Sebastien, I reviewed the patch and have questions about usage and defaults.
> 
> First, you modify default Queue size, which probably contributes to the
> regression failure in traffic-control module.  Can you find a solution that
> does not modify the network module?

Maybe in WifiMacQueue we can overwrite the default value of Queue? I will have a look.

> 
> Second, it seems that this feature is not default when 11ax is configured;
> should it be?

There is nothing mentioned about default in the standard since this is based on negotiation, so this will be vendor specific to decide what they enable by default.
My idea was to keep 64 as default.

> 
> It also seems that two steps are needed to enable this feature.
> 
> 1) change attribute HeConfiguration::MpduBufferSize from 64 to 256
> 2) change MaxAmpduSize in the MpduAggregator.
> 
> Regarding 2), the test code performs a low-level configuration that I would
> not expect users to have to do:
> 
>   m_mac->GetBEQueue ()->GetMpduAggregator ()->SetMaxAmpduSize (4194304);
> //Maximum size allowed by 802.11ax
> 
> However, the higher-level approach to this seems to be to change four
> RegularWifiMac attributes from 65535 to 4194304; each of:
> 
> BE_MaxAmpduSize
> BK_MaxAmpduSize
> VO_MaxAmpduSize
> VI_MaxAmpduSize
> 
> 
> In summary, I would prefer to either make the setting of the above 5
> attributes default in 11ax if this is the appropriate default, and if not, I
> would suggest to create a helper method that sets these five attributes.
> 
>   WifiHeHelper wifiHeHelper;
>   wifiHeHelper.EnableMaxAmpduAggregation ();
>   ...
> 
> (we haven't talked yet about a WifiHeHelper so the above API is provisional)

I do not think we need a specific helper, since this is redundant with the standard being configured. 

I guess we can increase default aggregation size based on the configured standard, but be aware that we have different default values based on AC (values you had received from a manufacturer if I'm right). VI_MaxAmpduSize and BE_MaxAmpduSize are set to 65535 (which by the way is 11n default, not 11ac default!), others are set to 0 (aggregation disabled).

> 
> Longer term, it would also help if there were some kind of example program
> using the preferred high-level API that contrasts and documents the maximum
> throughput available for 11n, 11ac, and 11ax (by showing by example how to
> tune these configurations to maximize throughput, and by stating what the
> ns-3 implementation of these standards is able to maximally achieve).

Maybe we could add a parameter in ht-wifi-example?

Update to my previous comment:

I see several solution to not touch the default Queue size.
Either set queue size in Txop, but then if user is setting default it will be overwritten (so this impact API somehow)
Another solution I see is to implement a virtual function GetDefaultMaxSize which will be overridden in WifiMacQueue, but this implies a (small) change in Queue implementation.
Please let me now what your opinion is.

> > (we haven't talked yet about a WifiHeHelper so the above API is provisional)
> 
> I do not think we need a specific helper, since this is redundant with the
> standard being configured. 

I didn't understand this comment because you said above that 64 would remain the  default.

If there is not a helper method, then multiple things have to be coordinated (queue depth, attributes, etc.) with a sequence of statements, right?

(In reply to sebastien.deronne from comment #5)
> Update to my previous comment:
> 
> I see several solution to not touch the default Queue size.
> Either set queue size in Txop, but then if user is setting default it will
> be overwritten (so this impact API somehow)
> Another solution I see is to implement a virtual function GetDefaultMaxSize
> which will be overridden in WifiMacQueue, but this implies a (small) change
> in Queue implementation.
> Please let me now what your opinion is.

I would prefer to avoid any changes to the Queue implementation.

(In reply to Tom Henderson from comment #7)
> (In reply to sebastien.deronne from comment #5)
> > Update to my previous comment:
> > 
> > I see several solution to not touch the default Queue size.
> > Either set queue size in Txop, but then if user is setting default it will
> > be overwritten (so this impact API somehow)
> > Another solution I see is to implement a virtual function GetDefaultMaxSize
> > which will be overridden in WifiMacQueue, but this implies a (small) change
> > in Queue implementation.
> > Please let me now what your opinion is.
> 
> I would prefer to avoid any changes to the Queue implementation.

(In reply to Tom Henderson from comment #6)
> > > (we haven't talked yet about a WifiHeHelper so the above API is provisional)
> > 
> > I do not think we need a specific helper, since this is redundant with the
> > standard being configured. 
> 
> I didn't understand this comment because you said above that 64 would remain
> the  default.
> 
> If there is not a helper method, then multiple things have to be coordinated
> (queue depth, attributes, etc.) with a sequence of statements, right?

I agree we might need to adapt wifi helper when 11ax is configured, but what should we use as default maximum aggregation size? We currently have 65535 for VI and BE, but those are maximum size in 11n, not even 11ac.

A possible option I see is keep BK and VO to 0 for all standards, and then for BE and VI set to the maximum value based on the configured standard. What do you think about that?

For aggregation sizes, I propose the following:

- move parameters from RegularWifiMac to each Configuration objects, with their corresponding maximum aggregation size.
- If HE station, regular wifi mac makes use of value in He Configuration, if VHT station, regular wifi mac makes use of value in Vht Configuration, and so on.

Created attachment 3218 [details]
Avoid change in Queue class

Wrt to the queue settings, I made a patch that allows to increase the Wifi queue size without impacting the Queue base class but still with the possibility to tune the Wifi Queue Size (but API is changed!). I do not see a better solution unless we agree to change the Queue base class, or that we simply increase the default value in the Queue base class.

Created attachment 3220 [details]
Improve aggregation API (+ various fixes)

I implemented my previous proposal.
This totally makes sense to me now to have different aggregation configurations for HT, VHT and HE as they have different maximum size which should be selected based on the modulation class used to transmit PSDU.
Aggregation sizes are now correct per standard and I also fixed an issue with the exponent advertised in HeCapabilities (+ other various fixes).

Now the configuration of 11ax aggregation is much more simple (see updated he-wifi-example).

However, I noticed a bug: the throughput is much lower than expected (even lower than with non-extended A-MPDU). At the beginning all works good (up to 256 MPDus are sent and successfully received) but there is a problem at a moment (this only occurs after a given simulation time). I will further investigate this week.

In the meantime, please let me know your opinion on the two added patches.

I probably will have some more comments later (it would be nice to have the patch on a pull request or Rietveld review) but for now would like to suggest:

1) As long as you are renaming attributes, can you avoid underscores in the name (e.g. VO_MaxAmsduSize)?

2) I'd like to suggest removing all of the setters and getters that are redundant with the built-in attribute setters and getters.  If the setter is doing something besides assigning the value, it can be kept.

(In reply to Tom Henderson from comment #12)
> I probably will have some more comments later (it would be nice to have the
> patch on a pull request or Rietveld review) but for now would like to
> suggest:

Ok, I'll use Rietveld unless we get Gitlab in place soon.

> 
> 1) As long as you are renaming attributes, can you avoid underscores in the
> name (e.g. VO_MaxAmsduSize)?

So you want VoMaxAmsduSize right?

> 
> 2) I'd like to suggest removing all of the setters and getters that are
> redundant with the built-in attribute setters and getters.  If the setter is
> doing something besides assigning the value, it can be kept.


OK. I found the code cleaner when fetching using a getter instead of the built-in attribute system, but this is a personal opinion, I agree this leads to extra code that can be avoided.

Note that I found the problem and I am busy on a fix, I'll post it later today or tomorrow at the very latest.

Hello Sébastien, Tom,
Is there a reason why HtConfiguration, VhtConfiguration, and HeConfiguration could not have a common parent class specifying queue specific attributes and methods? Is it because the attribute system cannot define per-child specific max values?
Rediet

Created attachment 3224 [details]
Fix remaining issues

I managed to fix remaining issues, all tests are passing and I added an extra test showing an increased throughput when using extended A-MPDU (we overpass 1 Gbit/s in the example at highest PHY rate without MIMO).

Note that I had to increase the timeout value for 11ax, I have not seen anything mentioned in the standard.

I will provide a final patch once I get feedback from Tom (and eventually others) about this and about open comments (renaming with or without underscore, keep or remove setters/getters, ...)

(In reply to Rediet from comment #14)
> Hello Sébastien, Tom,
> Is there a reason why HtConfiguration, VhtConfiguration, and HeConfiguration
> could not have a common parent class specifying queue specific attributes
> and methods? Is it because the attribute system cannot define per-child
> specific max values?
> Rediet

I also had that idea in mind, but boundary checks are different according to the standard so I would not go for this.

> 
> I will provide a final patch once I get feedback from Tom (and eventually
> others) about this and about open comments (renaming with or without
> underscore, keep or remove setters/getters, ...)

I am OK with this part of the patch.

(In reply to Tom Henderson from comment #17)
> > 
> > I will provide a final patch once I get feedback from Tom (and eventually
> > others) about this and about open comments (renaming with or without
> > underscore, keep or remove setters/getters, ...)
> 
> I am OK with this part of the patch.

Tom still two open points:
1/ Do we remove _ in names? I am actually fine with this proposal
2/ Do we remove setters/getters? I find that less user-friendly.

> 
> Tom still two open points:
> 1/ Do we remove _ in names? I am actually fine with this proposal

Yes, I favor removing underscores.

> 2/ Do we remove setters/getters? I find that less user-friendly.

How about the following informal policy?  If users are going to be handling the configuration in user-level code, provide the setter and getter.  If the setter has side effects, of course, provide it then.  If users are most likely going to be using config paths or defaults to change the value of a given attribute, and the only code needing the setter/getter is low-level code between objects, use the built-in.

Created attachment 3231 [details]
latest proposal

I merged previous patches and handled review comments. I would like to push this to the mainstream still this week.

Patch pushed and regression fixed in changeset 13869:7c1901e04f94