| Message ID | 20181105143027.17570-1-sergey.matyukevich.os@quantenna.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | cfg80211/nl80211/iw: add basic AMPDU/AMSDU controls | expand |
Hi, > Here are several RFC patches providing simple high-level controls of AMSDU/AMPDU > aggregation. The primary purpose of this functionality is an attempt to fill > missing gaps in nl80211 interface for basic WFA certification tests. I see you don't implement it this way in the driver, but wouldn't it make more sense to have this as a per-STA (RA) setting? That's really the granularity it can be done on, I think? Arguably even per-RA/TID, though that seems a little excessive? > We experimented with QCA sigma-dut tool: https://github.com/qca/sigma-dut. > The purpose was to cover basic HT/VHT WFA STA tests for cfg80211 driver > controlled by wpa_supplicant w/o adding any vendor specific commands. > Multiple WFA test parameters (e.g. STBC, NSS, SGI, LDPC) can be configured > by overriding HT/VHT capabilities in wpa_supplicant and applying them on > connect in cfg80211_connect callback. Others (e.g. RTS params) can be > configured using iw tool or NL80211_CMD_SET_WIPHY directly. These patches > implement simpe high-level switches for AMSDU/AMPDU aggregation. > > It would be interesting to collect comments/concerns regarding this approach. > Does it make sense to enhance nl80211 in order to cover all the missing pieces > required for WFA certification tests ? Or maybe it makes sense to use > NL80211_TESTMODE subcommands for this kind of testing. Honestly, I don't really know. I think other tests, e.g. noack, we used to just have debugfs files for, and then we got NL80211_CMD_SET_NOACK_MAP (which *is* per TID, btw) Perhaps if we really don't see any value beyond the testing, keeping it in debugfs would make sense, until we have more use cases and understand the granularity we should support? Clearly this is enough for the testing you refer to, but other use cases might actually need more fine-grained control (in the future), possibly down to the RA/TID level? johannes
On 11/05/2018 12:32 PM, Johannes Berg wrote: > Hi, > >> Here are several RFC patches providing simple high-level controls of AMSDU/AMPDU >> aggregation. The primary purpose of this functionality is an attempt to fill >> missing gaps in nl80211 interface for basic WFA certification tests. > > I see you don't implement it this way in the driver, but wouldn't it > make more sense to have this as a per-STA (RA) setting? That's really > the granularity it can be done on, I think? > > Arguably even per-RA/TID, though that seems a little excessive? I like the idea of providing this API per peer/tid. And, just allow peer == -1, tid == -1 or similar to mean 'all' so that you can still set the entire device to one particular setting w/out having to iterate through all peers if you don't want to iterate... >> We experimented with QCA sigma-dut tool: https://github.com/qca/sigma-dut. >> The purpose was to cover basic HT/VHT WFA STA tests for cfg80211 driver >> controlled by wpa_supplicant w/o adding any vendor specific commands. >> Multiple WFA test parameters (e.g. STBC, NSS, SGI, LDPC) can be configured >> by overriding HT/VHT capabilities in wpa_supplicant and applying them on >> connect in cfg80211_connect callback. Others (e.g. RTS params) can be >> configured using iw tool or NL80211_CMD_SET_WIPHY directly. These patches >> implement simpe high-level switches for AMSDU/AMPDU aggregation. >> >> It would be interesting to collect comments/concerns regarding this approach. >> Does it make sense to enhance nl80211 in order to cover all the missing pieces >> required for WFA certification tests ? Or maybe it makes sense to use >> NL80211_TESTMODE subcommands for this kind of testing. > > Honestly, I don't really know. > > I think other tests, e.g. noack, we used to just have debugfs files for, > and then we got NL80211_CMD_SET_NOACK_MAP (which *is* per TID, btw) > > Perhaps if we really don't see any value beyond the testing, keeping it > in debugfs would make sense, until we have more use cases and understand > the granularity we should support? > > Clearly this is enough for the testing you refer to, but other use cases > might actually need more fine-grained control (in the future), possibly > down to the RA/TID level? At least with ath10k, it seems to be a common struggle for AP manufacturers to do regulatory testing. I would image that is true for other chipsets as well, so it seems like it might be worth adding to the official API. Thanks, Ben
On 11/05/2018 12:45 PM, Ben Greear wrote: >> >> I see you don't implement it this way in the driver, but wouldn't it >> make more sense to have this as a per-STA (RA) setting? That's really >> the granularity it can be done on, I think? >> >> Arguably even per-RA/TID, though that seems a little excessive? > > I like the idea of providing this API per peer/tid. And, just allow > peer == -1, tid == -1 or similar to mean 'all' so that you can still set > the entire device > to one particular setting w/out having to iterate through all peers if you > don't want to iterate... Maye I'm wrong, but isn't the setting we're discussing are for the device itself, not for its peers? I mean, disabling AMSDU, AMPDU implies we need to update capabilities advertised in our information elements, which are common for all devices, and it affects both Tx and Rx. And per-node/per-TID aggregation settings are a separate configuration option related to rate adaptation on Tx path only..
On 11/05/2018 12:45 PM, Ben Greear wrote: >> >> I see you don't implement it this way in the driver, but wouldn't it >> make more sense to have this as a per-STA (RA) setting? That's really >> the granularity it can be done on, I think? >> >> Arguably even per-RA/TID, though that seems a little excessive? > > I like the idea of providing this API per peer/tid. And, just allow > peer == -1, tid == -1 or similar to mean 'all' so that you can still set > the entire device > to one particular setting w/out having to iterate through all peers if you > don't want to iterate... Maybe I'm wrong, but isn't the setting we're discussing are for the device itself, not for its peers? I mean, disabling AMSDU, AMPDU implies we need to update capabilities advertised in our information elements, which are common for all devices, and it affects both Tx and Rx. And per-node/per-TID aggregation settings are a separate configuration option related to rate adaptation on Tx path only..
On 11/05/2018 02:49 PM, Igor Mitsyanko wrote: > On 11/05/2018 12:45 PM, Ben Greear wrote: >>> >>> I see you don't implement it this way in the driver, but wouldn't it >>> make more sense to have this as a per-STA (RA) setting? That's really >>> the granularity it can be done on, I think? >>> >>> Arguably even per-RA/TID, though that seems a little excessive? >> >> I like the idea of providing this API per peer/tid. And, just allow >> peer == -1, tid == -1 or similar to mean 'all' so that you can still set >> the entire device >> to one particular setting w/out having to iterate through all peers if you >> don't want to iterate... > > Maye I'm wrong, but isn't the setting we're discussing are for the > device itself, not for its peers? I mean, disabling AMSDU, AMPDU implies > we need to update capabilities advertised in our information elements, > which are common for all devices, and it affects both Tx and Rx. > > And per-node/per-TID aggregation settings are a separate configuration > option related to rate adaptation on Tx path only.. You can advertise your maximum capabilities, but just because you advertise that you can do large AMPDU chains doesn't mean you are required to send them. So, to advertise stuff, it is per vdev (not per radio), but once you associate a peer, you might decide to configure it so that you always send no more than 5 frames in an AMPDU chain, for instance. And, you might decide that BE gets up to 32 AMPDU chain, but VI should be limitted to 16 to decrease latency just a bit. Thanks, Ben
> On 11/05/2018 02:49 PM, Igor Mitsyanko wrote: > > On 11/05/2018 12:45 PM, Ben Greear wrote: > > > > > > > > I see you don't implement it this way in the driver, but wouldn't it > > > > make more sense to have this as a per-STA (RA) setting? That's really > > > > the granularity it can be done on, I think? > > > > > > > > Arguably even per-RA/TID, though that seems a little excessive? > > > > > > I like the idea of providing this API per peer/tid. And, just allow > > > peer == -1, tid == -1 or similar to mean 'all' so that you can still set > > > the entire device > > > to one particular setting w/out having to iterate through all peers if you > > > don't want to iterate... > > > > Maye I'm wrong, but isn't the setting we're discussing are for the > > device itself, not for its peers? I mean, disabling AMSDU, AMPDU implies > > we need to update capabilities advertised in our information elements, > > which are common for all devices, and it affects both Tx and Rx. > > > > And per-node/per-TID aggregation settings are a separate configuration > > option related to rate adaptation on Tx path only.. > > You can advertise your maximum capabilities, but just because you advertise > that you can do large AMPDU chains doesn't mean you are required to send > them. > > So, to advertise stuff, it is per vdev (not per radio), but once you associate > a peer, you might decide to configure it so that you always send no more than 5 > frames in an AMPDU chain, for instance. > > And, you might decide that BE gets up to 32 AMPDU chain, but VI should be limitted to 16 > to decrease latency just a bit. Hi all, Thanks for the comments. It turns out that RA/TID-aware approach to AMPDU configuration has been already posted. Johannes pointed me at the patch set adding support for TID specific configuration: https://patchwork.kernel.org/project/linux-wireless/list/?series=33855 That patch set enables per-TID and per-STA configuration. AFAICS it can be easily extended to support AMSDU and even configurable AMPDU chain depth. Regards, Sergey