Issue 215470043: 802.11n hybrid aggregation
|
Created:
10 years, 1 month ago by S. Deronne Modified:
10 years ago CC:
ns-3-reviews_googlegroups.com Visibility:
Public. |
Description802.11n hybrid aggregation
Patch Set 1 #
Total comments: 16
MessagesTotal messages: 18
My inline comments ask about three issues: - can the public API be simplified? - can we avoid adding base class methods to MacLowTransmissionListener? - more comments in the example I'd also like to add: - the term 'hybrid aggregation' seems to be non-standard; I looked through the 2012 standard and 'hybrid' seems to refer only to DSSS-OFDM and HCF/HCCA, so can a different term be used, to avoid possible confusion? - can you add something about this feature in the wifi.rst? - could a test of this code be added (is it possible to unit test the aggregation code?) https://codereview.appspot.com/215470043/diff/1/examples/wireless/simple-hybr... File examples/wireless/simple-hybrid-aggregation.cc (right): https://codereview.appspot.com/215470043/diff/1/examples/wireless/simple-hybr... examples/wireless/simple-hybrid-aggregation.cc:47: // belonging to BestEffort Access Class (AC_BE). can you add here some comments about what users should expect to see as they increase both parameters? The program will output a throughput; presumably, as users increase nMsdus and nMpdus, the throughput will increase. At what point of increasing are there diminishing returns in this example, and why? Explain also what happens if users increase only one but not both of the arguments, and the sensitivity to payloadSize. Comment also on what rate control will be used. https://codereview.appspot.com/215470043/diff/1/examples/wireless/simple-hybr... examples/wireless/simple-hybrid-aggregation.cc:70: Config::SetDefault ("ns3::WifiRemoteStationManager::RtsCtsThreshold", StringValue ("999999")); I suggest to run 'utils/check-style.py -i -f examples/wireless/simple-hybrid-aggregation.cc -l 3' on this file (and similarly on other new files that you introduce); it will fix lack of braces, indentation style, etc. https://codereview.appspot.com/215470043/diff/1/examples/wireless/simple-hybr... examples/wireless/simple-hybrid-aggregation.cc:99: //Enable hybrid aggregation at the STA side The API below looks a bit more complicated than it ought to be (compare with the API immediately above). Is that an artifact of manipulating the nMpdus and nMsdus at a low level, or is it this complicated even when running with defaults? Is hybrid aggregation enabled by default when HtWifiMacHelper is used? (why or why not?) Why can't these parameters be exposed as attributes like other parameters above, such as: mac.SetType ("ns3::StaWifiMac", "Ssid", SsidValue (ssid), "ActiveProbing", BooleanValue (false), "MaxAmpduSize", UintegerValue (...), "BlockAckThreshold", UintegerValue (2), ...); (this is a general comment on the API for MSDU and MPDU aggregation at the helper level) https://codereview.appspot.com/215470043/diff/1/src/wifi/model/mac-low.cc File src/wifi/model/mac-low.cc (right): https://codereview.appspot.com/215470043/diff/1/src/wifi/model/mac-low.cc#new... src/wifi/model/mac-low.cc:2697: { inconsistent use of brace indenting throughout the patch (use two spaces) https://codereview.appspot.com/215470043/diff/1/src/wifi/model/mac-low.h File src/wifi/model/mac-low.h (right): https://codereview.appspot.com/215470043/diff/1/src/wifi/model/mac-low.h#newc... src/wifi/model/mac-low.h:139: virtual Ptr<MsduAggregator> GetMsduAggregator (void); const? https://codereview.appspot.com/215470043/diff/1/src/wifi/model/mac-low.h#newc... src/wifi/model/mac-low.h:142: virtual Mac48Address MapSrcAddressForAggregation (const WifiMacHeader &hdr); High level comment: this is the kind of design that we tried to get away from by using ns3::Object architecture. Here, you are forced to implement inappropriate methods for the base class that return invalid values because you are trying to use a base class pointer to get at subclass functionality. In your client code, you have to check for these invalid values such as (if m_listener->GetMsduAggregator () != 0). Is it possible to convert MacLowTransmissionListener to derive from ns3::Object or ns3::SimpleRefCount? Unless attributes are needed, I might recommend the latter. This would then allow you to avoid these base class methods and instead in the client code, you can use instead of: if (m_listener->GetMsduAggregator () != 0) this instead Ptr<EdcaTxopN::TransmissionListener> m_edcaListener = DynamicCast<EdcaTxopN::TransmissionListener> (m_listener); if (m_edcaListener) while the above is a bit more typing in the client code, it is IMO more readable as to what the code is checking for, and avoids having to add public API to MacLow. (if the base class were ns3::Object(), one could also use GetObject<EdcaTxopN::TransmissionListener> (m_listener); to accomplish the same fetch of the derived class interface). If HCCA (in the future) is intended to support this, then perhaps it is a bit more complicated (either an intermediate base class for both Edca and Hcca is needed with this interface, or GetObject<> can be used), but in general, there ought to be solutions that avoid these methods in the base class that don't pertain to DcaTxop listeners. https://codereview.appspot.com/215470043/diff/1/src/wifi/model/mac-low.h#newc... src/wifi/model/mac-low.h:1255: */ missing doxygen
Sign in to reply to this message.
https://codereview.appspot.com/215470043/diff/1/src/wifi/model/mac-low.cc File src/wifi/model/mac-low.cc (right): https://codereview.appspot.com/215470043/diff/1/src/wifi/model/mac-low.cc#new... src/wifi/model/mac-low.cc:2697: { On 2015/03/21 16:57:11, Tom Henderson wrote: > inconsistent use of brace indenting throughout the patch (use two spaces) Done.
Sign in to reply to this message.
https://codereview.appspot.com/215470043/diff/1/src/wifi/model/mac-low.h File src/wifi/model/mac-low.h (right): https://codereview.appspot.com/215470043/diff/1/src/wifi/model/mac-low.h#newc... src/wifi/model/mac-low.h:1255: */ On 2015/03/21 16:57:11, Tom Henderson wrote: > missing doxygen Done: /** * Perform MSDU aggregation for a given MPDU in an A-MPDU * * \param packet packet picked for aggregation * \param hdr 802.11 header for packet picked for aggregation * \param tstamp timestamp * \param currentAmpduPacket current A-MPDU packet * \param blockAckSize size of the piggybacked block ack request * * \return the aggregate if MSDU aggregation succeeded, 0 otherwise */
Sign in to reply to this message.
https://codereview.appspot.com/215470043/diff/1/examples/wireless/simple-hybr... File examples/wireless/simple-hybrid-aggregation.cc (right): https://codereview.appspot.com/215470043/diff/1/examples/wireless/simple-hybr... examples/wireless/simple-hybrid-aggregation.cc:70: Config::SetDefault ("ns3::WifiRemoteStationManager::RtsCtsThreshold", StringValue ("999999")); On 2015/03/21 16:57:11, Tom Henderson wrote: > I suggest to run 'utils/check-style.py -i -f > examples/wireless/simple-hybrid-aggregation.cc -l 3' on this file (and similarly > on other new files that you introduce); it will fix lack of braces, indentation > style, etc. Done.
Sign in to reply to this message.
On 2015/03/21 16:57:11, Tom Henderson wrote: > - the term 'hybrid aggregation' seems to be non-standard; I looked through the > 2012 standard and 'hybrid' seems to refer only to DSSS-OFDM and HCF/HCCA, so can > a different term be used, to avoid possible confusion? The standard explicitly defines A-MSDU and A-MPDU, and both can be combined together. This is generally denoted as "hybrid" aggregation or "two-level" aggregation, but it is not strictly mentioned in the standard. Do you prefer the term two-level aggregation for this feature? > Is hybrid aggregation enabled by default when HtWifiMacHelper is used? (why or > why not?) The Wi-Fi Alliance 802.11n certification program requires support for reception of A-MPDU and A-MSDU frames, transmission is optional. So, it is not (and should not be) enabled by default in the helper.
Sign in to reply to this message.
> Why can't these parameters be exposed as attributes like other parameters
above,
> such as:
>
> mac.SetType ("ns3::StaWifiMac",
> "Ssid", SsidValue (ssid),
> "ActiveProbing", BooleanValue (false),
> "MaxAmpduSize", UintegerValue (...),
> "BlockAckThreshold", UintegerValue (2),
> ...);
>
> (this is a general comment on the API for MSDU and MPDU aggregation at the
> helper level)
This is because aggregation-related parameters are set per AC.
I kept the same implementation as done for A-MSDU and A-MPDU.
Sign in to reply to this message.
https://codereview.appspot.com/215470043/diff/1/src/wifi/model/mac-low.cc File src/wifi/model/mac-low.cc (right): https://codereview.appspot.com/215470043/diff/1/src/wifi/model/mac-low.cc#new... src/wifi/model/mac-low.cc:2872: WifiMacHeader::ADDR1, hdr->GetAddr1 (), tstamp); shouldn't the while loop check if the sequence number of the packet being aggregated is inside the window before aggregating so that the code doesn't send more than 64 packets? I see the sequence number is checked before this function is called but this condition (64 packets can be outstanding) can get violated here?
Sign in to reply to this message.
https://codereview.appspot.com/215470043/diff/1/src/wifi/model/mac-low.cc File src/wifi/model/mac-low.cc (right): https://codereview.appspot.com/215470043/diff/1/src/wifi/model/mac-low.cc#new... src/wifi/model/mac-low.cc:2872: WifiMacHeader::ADDR1, hdr->GetAddr1 (), tstamp); On 2015/04/06 17:57:04, gbadawy wrote: > shouldn't the while loop check if the sequence number of the packet being > aggregated is inside the window before aggregating so that the code doesn't send > more than 64 packets? I see the sequence number is checked before this function > is called but this condition (64 packets can be outstanding) can get violated > here? This check is done upfront in MacLow::AggregateToAmpdu: if (m_listener->GetMsduAggregator () != 0 && IsInWindow (currentSequenceNumber, startingSequenceNumber, 64)) This is used to avoid packets to be removed if they can not be aggregated in the A-MPDU.
Sign in to reply to this message.
> - can we avoid adding base class methods to MacLowTransmissionListener? It has been decided to rename BlockAckEventListener to AggregationCapableTransmissionListener, and to move those methods from MacLowTransmissionListener to AggregationCapableTransmissionListener.
Sign in to reply to this message.
On Mon, Apr 6, 2015 at 4:16 PM, <sebastien.deronne@gmail.com> wrote: > > https://codereview.appspot.com/215470043/diff/1/src/wifi/model/mac-low.cc > File src/wifi/model/mac-low.cc (right): > > https://codereview.appspot.com/215470043/diff/1/src/wifi/ > model/mac-low.cc#newcode2872 > src/wifi/model/mac-low.cc:2872: WifiMacHeader::ADDR1, hdr->GetAddr1 (), > tstamp); > On 2015/04/06 17:57:04, gbadawy wrote: > >> shouldn't the while loop check if the sequence number of the packet >> > being > >> aggregated is inside the window before aggregating so that the code >> > doesn't send > >> more than 64 packets? I see the sequence number is checked before this >> > function > >> is called but this condition (64 packets can be outstanding) can get >> > violated > >> here? >> > > This check is done upfront in MacLow::AggregateToAmpdu: > if (m_listener->GetMsduAggregator () != 0 && IsInWindow > (currentSequenceNumber, startingSequenceNumber, 64)) That is not enough you make sure the first one is in the window. how about the rest that are aggregated inside PerformMsduAggregation and are removed from the queue there? There is no check done on them inside the function > This is used to avoid packets to be removed if they can not be > aggregated in the A-MPDU. > > https://codereview.appspot.com/215470043/ >
Sign in to reply to this message.
On 2015/04/09 13:36:02, gbadawy wrote: > On Mon, Apr 6, 2015 at 4:16 PM, <mailto:sebastien.deronne@gmail.com> wrote: > > > > > https://codereview.appspot.com/215470043/diff/1/src/wifi/model/mac-low.cc > > File src/wifi/model/mac-low.cc (right): > > > > https://codereview.appspot.com/215470043/diff/1/src/wifi/ > > model/mac-low.cc#newcode2872 > > src/wifi/model/mac-low.cc:2872: WifiMacHeader::ADDR1, hdr->GetAddr1 (), > > tstamp); > > On 2015/04/06 17:57:04, gbadawy wrote: > > > >> shouldn't the while loop check if the sequence number of the packet > >> > > being > > > >> aggregated is inside the window before aggregating so that the code > >> > > doesn't send > > > >> more than 64 packets? I see the sequence number is checked before this > >> > > function > > > >> is called but this condition (64 packets can be outstanding) can get > >> > > violated > > > >> here? > >> > > > > This check is done upfront in MacLow::AggregateToAmpdu: > > if (m_listener->GetMsduAggregator () != 0 && IsInWindow > > (currentSequenceNumber, startingSequenceNumber, 64)) > > > That is not enough you make sure the first one is in the window. how about > the rest that are aggregated inside PerformMsduAggregation and are removed > from the queue there? There is no check done on them inside the function > > > This is used to avoid packets to be removed if they can not be > > aggregated in the A-MPDU. > > > > > > > https://codereview.appspot.com/215470043/ > > Ghada, since all packets within the A-MSDU have the same sequence number, the check on the first packet is enough.
Sign in to reply to this message.
On Thu, Apr 9, 2015 at 12:05 PM, <sebastien.deronne@gmail.com> wrote: > On 2015/04/09 13:36:02, gbadawy wrote: > >> On Mon, Apr 6, 2015 at 4:16 PM, <mailto:sebastien.deronne@gmail.com> >> > wrote: > > > >> > >> > https://codereview.appspot.com/215470043/diff/1/src/wifi/model/mac-low.cc > >> > File src/wifi/model/mac-low.cc (right): >> > >> > https://codereview.appspot.com/215470043/diff/1/src/wifi/ >> > model/mac-low.cc#newcode2872 >> > src/wifi/model/mac-low.cc:2872: WifiMacHeader::ADDR1, hdr->GetAddr1 >> > (), > >> > tstamp); >> > On 2015/04/06 17:57:04, gbadawy wrote: >> > >> >> shouldn't the while loop check if the sequence number of the packet >> >> >> > being >> > >> >> aggregated is inside the window before aggregating so that the code >> >> >> > doesn't send >> > >> >> more than 64 packets? I see the sequence number is checked before >> > this > >> >> >> > function >> > >> >> is called but this condition (64 packets can be outstanding) can >> > get > >> >> >> > violated >> > >> >> here? >> >> >> > >> > This check is done upfront in MacLow::AggregateToAmpdu: >> > if (m_listener->GetMsduAggregator () != 0 && IsInWindow >> > (currentSequenceNumber, startingSequenceNumber, 64)) >> > > > That is not enough you make sure the first one is in the window. how >> > about > >> the rest that are aggregated inside PerformMsduAggregation and are >> > removed > >> from the queue there? There is no check done on them inside the >> > function > > > This is used to avoid packets to be removed if they can not be >> > aggregated in the A-MPDU. >> > > > > > >> > https://codereview.appspot.com/215470043/ >> > >> > > Ghada, since all packets within the A-MSDU have the same sequence > number, the check on the first packet is enough. > I know maybe I didn't explain myself clearly but in reality these MSDUs are held in the receiver re-order buffer (which is usually limited in size) so it doesn't matter if you send single MSDUs or A-MSDUs the size of packets that a receiver buffer can hold is constant and not dependent on the sequence number but on the packet size, right? Before 2 level aggregation the user knows the size of this buffer (packet size *64) but now with 2 level aggregation that is not true (because it varies from packet size * 64 till A-MSDU size *64). Since a lot of users use ns3 to simulate real devices I think this might be an issue now. What do you think, should we start taking buffer size into consideration and check for it we can have a defaults value of (A-MSDU size*64) but the user should be able to adjust it to lower and notice a different behaviour? > https://codereview.appspot.com/215470043/ >
Sign in to reply to this message.
> I know maybe I didn't explain myself clearly but in reality these MSDUs are > held in the receiver re-order buffer (which is usually limited in size) so > it doesn't matter if you send single MSDUs or A-MSDUs the size of packets > that a receiver buffer can hold is constant and not dependent on the > sequence number but on the packet size, right? Before 2 level aggregation > the user knows the size of this buffer (packet size *64) but now with 2 > level aggregation that is not true (because it varies from packet size * 64 > till A-MSDU size *64). Since a lot of users use ns3 to simulate real > devices I think this might be an issue now. What do you think, should we > start taking buffer size into consideration and check for it we can have a > defaults value of (A-MSDU size*64) but the user should be able to adjust it > to lower and notice a different behaviour? What is the current maximum buffer size? In my tests I have not noticed any buffer issues, but maybe some special scenarios have not been covered. Do you have an example in mind which would result in a buffer issue
Sign in to reply to this message.
- can you add something about this feature in the wifi.rst? Will be updated in patch set 2.
Sign in to reply to this message.
On 2015/04/11 09:09:17, S. Deronne wrote: > > I know maybe I didn't explain myself clearly but in reality these MSDUs are > > held in the receiver re-order buffer (which is usually limited in size) so > > it doesn't matter if you send single MSDUs or A-MSDUs the size of packets > > that a receiver buffer can hold is constant and not dependent on the > > sequence number but on the packet size, right? Before 2 level aggregation > > the user knows the size of this buffer (packet size *64) but now with 2 > > level aggregation that is not true (because it varies from packet size * 64 > > till A-MSDU size *64). Since a lot of users use ns3 to simulate real > > devices I think this might be an issue now. What do you think, should we > > start taking buffer size into consideration and check for it we can have a > > defaults value of (A-MSDU size*64) but the user should be able to adjust it > > to lower and notice a different behaviour? > > What is the current maximum buffer size? > In my tests I have not noticed any buffer issues, but maybe some special > scenarios have not been covered. > Do you have an example in mind which would result in a buffer issue Ghada suggests to not bother about buffer size here since ns-3 doesn't take buffer size into consideration yet. Note: the receiver buffer size should be negotitated in the AddBa.
Sign in to reply to this message.
On 2015/03/21 16:57:11, Tom Henderson wrote: > My inline comments ask about three issues: > > - can the public API be simplified? > - can we avoid adding base class methods to MacLowTransmissionListener? > - could a test of this code be added (is it possible to unit test the > aggregation code?) Unit tests have been added for two-level aggregation. Note that those tests could be later extended, in particular to cover methods used for MPDU aggregation.
Sign in to reply to this message.
https://codereview.appspot.com/215470043/diff/1/examples/wireless/simple-hybr... File examples/wireless/simple-hybrid-aggregation.cc (right): https://codereview.appspot.com/215470043/diff/1/examples/wireless/simple-hybr... examples/wireless/simple-hybrid-aggregation.cc:47: // belonging to BestEffort Access Class (AC_BE). Some comments have been added. https://codereview.appspot.com/215470043/diff/1/examples/wireless/simple-hybr... examples/wireless/simple-hybrid-aggregation.cc:99: //Enable hybrid aggregation at the STA side Aggregation-related parameters are set per AC. This would require to refactor how QoS-based parameters are set -> could be discussed for a future improvement!
Sign in to reply to this message.
https://codereview.appspot.com/215470043/diff/1/src/wifi/model/mac-low.h File src/wifi/model/mac-low.h (right): https://codereview.appspot.com/215470043/diff/1/src/wifi/model/mac-low.h#newc... src/wifi/model/mac-low.h:139: virtual Ptr<MsduAggregator> GetMsduAggregator (void); On 2015/03/21 16:57:11, Tom Henderson wrote: > const? Done. https://codereview.appspot.com/215470043/diff/1/src/wifi/model/mac-low.h#newc... src/wifi/model/mac-low.h:142: virtual Mac48Address MapSrcAddressForAggregation (const WifiMacHeader &hdr); I let this as a future improvement since it is not only involving two-level aggregation changes.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
