| Message ID | 1378752545-9783-1-git-send-email-greearb@candelatech.com |
|---|---|
| State | Rejected |
| Headers | show |
On Mon, Sep 09, 2013 at 11:49:04AM -0700, greearb@candelatech.com wrote: > Without this patch, wpa_supplicant EAPOL packets (at least) > are sent on normal best-effort TX queue. I believe they > should be on the VO high-priority queue instead. > diff --git a/src/l2_packet/l2_packet_linux.c b/src/l2_packet/l2_packet_linux.c > @@ -97,6 +97,7 @@ struct l2_packet_data * l2_packet_init( > + /* Use high-priority queue for management packets > + * http://wireless.kernel.org/en/developers/Documentation/mac80211/queues > + */ > + if (setsockopt(l2->fd, SOL_SOCKET, > + SO_PRIORITY, (char*)&val, sizeof(val)) < 0) { This seems to be assuming that mac80211 is used always. That is not really the case and I guess this SO_PRIORITY change to a special mac80211-specific value could result in undesired results with non-mac80211 drivers.
On 09/23/2013 01:08 AM, Jouni Malinen wrote: > On Mon, Sep 09, 2013 at 11:49:04AM -0700, greearb@candelatech.com wrote: >> Without this patch, wpa_supplicant EAPOL packets (at least) >> are sent on normal best-effort TX queue. I believe they >> should be on the VO high-priority queue instead. > >> diff --git a/src/l2_packet/l2_packet_linux.c b/src/l2_packet/l2_packet_linux.c > >> @@ -97,6 +97,7 @@ struct l2_packet_data * l2_packet_init( >> + /* Use high-priority queue for management packets >> + * http://wireless.kernel.org/en/developers/Documentation/mac80211/queues >> + */ >> + if (setsockopt(l2->fd, SOL_SOCKET, >> + SO_PRIORITY, (char*)&val, sizeof(val)) < 0) { > > This seems to be assuming that mac80211 is used always. That is not > really the case and I guess this SO_PRIORITY change to a special > mac80211-specific value could result in undesired results with > non-mac80211 drivers. The special priority is defined in the net/wireless/ dir of the kernel, and does not make special mention of mac80211 v/s other logic. From what I can tell, there is no other way to specify QoS when sending on packet-sockets. The skb->priority never hits the wire directly, so either something should be using it properly, or it will just be ignored and we are no worse off than before. If some driver is actually conflicting with the documentation, then we just need to fix it. from net/wireless/util.c: /* Given a data frame determine the 802.1p/1d tag to use. */ unsigned int cfg80211_classify8021d(struct sk_buff *skb) { unsigned int dscp; /* skb->priority values from 256->263 are magic values to * directly indicate a specific 802.1d priority. This is used * to allow 802.1d priority to be passed directly in from VLAN * tags, etc. */ if (skb->priority >= 256 && skb->priority <= 263) return skb->priority - 256; switch (skb->protocol) { case htons(ETH_P_IP): dscp = ipv4_get_dsfield(ip_hdr(skb)) & 0xfc; break; case htons(ETH_P_IPV6): dscp = ipv6_get_dsfield(ipv6_hdr(skb)) & 0xfc; break; default: return 0; } return dscp >> 5; } EXPORT_SYMBOL(cfg80211_classify8021d); Thanks, Ben
Any more thought on this? On 09/23/2013 09:04 AM, Ben Greear wrote: > On 09/23/2013 01:08 AM, Jouni Malinen wrote: >> On Mon, Sep 09, 2013 at 11:49:04AM -0700, greearb@candelatech.com wrote: >>> Without this patch, wpa_supplicant EAPOL packets (at least) >>> are sent on normal best-effort TX queue. I believe they >>> should be on the VO high-priority queue instead. >> >>> diff --git a/src/l2_packet/l2_packet_linux.c b/src/l2_packet/l2_packet_linux.c >> >>> @@ -97,6 +97,7 @@ struct l2_packet_data * l2_packet_init( >>> + /* Use high-priority queue for management packets >>> + * http://wireless.kernel.org/en/developers/Documentation/mac80211/queues >>> + */ >>> + if (setsockopt(l2->fd, SOL_SOCKET, >>> + SO_PRIORITY, (char*)&val, sizeof(val)) < 0) { >> >> This seems to be assuming that mac80211 is used always. That is not >> really the case and I guess this SO_PRIORITY change to a special >> mac80211-specific value could result in undesired results with >> non-mac80211 drivers. > > The special priority is defined in the net/wireless/ dir of the kernel, > and does not make special mention of mac80211 v/s other logic. > > From what I can tell, there is no other way to specify QoS when > sending on packet-sockets. The skb->priority never hits the wire > directly, so either something should be using it properly, or it > will just be ignored and we are no worse off than before. If > some driver is actually conflicting with the documentation, then > we just need to fix it. > > from net/wireless/util.c: > > /* Given a data frame determine the 802.1p/1d tag to use. */ > unsigned int cfg80211_classify8021d(struct sk_buff *skb) > { > unsigned int dscp; > > /* skb->priority values from 256->263 are magic values to > * directly indicate a specific 802.1d priority. This is used > * to allow 802.1d priority to be passed directly in from VLAN > * tags, etc. > */ > if (skb->priority >= 256 && skb->priority <= 263) > return skb->priority - 256; > > switch (skb->protocol) { > case htons(ETH_P_IP): > dscp = ipv4_get_dsfield(ip_hdr(skb)) & 0xfc; > break; > case htons(ETH_P_IPV6): > dscp = ipv6_get_dsfield(ipv6_hdr(skb)) & 0xfc; > break; > default: > return 0; > } > > return dscp >> 5; > } > EXPORT_SYMBOL(cfg80211_classify8021d); > > > Thanks, > Ben >
On Mon, 2013-09-23 at 09:04 -0700, Ben Greear wrote: > On 09/23/2013 01:08 AM, Jouni Malinen wrote: > > On Mon, Sep 09, 2013 at 11:49:04AM -0700, greearb@candelatech.com wrote: > >> Without this patch, wpa_supplicant EAPOL packets (at least) > >> are sent on normal best-effort TX queue. I believe they > >> should be on the VO high-priority queue instead. > > > >> diff --git a/src/l2_packet/l2_packet_linux.c b/src/l2_packet/l2_packet_linux.c > > > >> @@ -97,6 +97,7 @@ struct l2_packet_data * l2_packet_init( > >> + /* Use high-priority queue for management packets > >> + * http://wireless.kernel.org/en/developers/Documentation/mac80211/queues > >> + */ > >> + if (setsockopt(l2->fd, SOL_SOCKET, > >> + SO_PRIORITY, (char*)&val, sizeof(val)) < 0) { > > > > This seems to be assuming that mac80211 is used always. That is not > > really the case and I guess this SO_PRIORITY change to a special > > mac80211-specific value could result in undesired results with > > non-mac80211 drivers. > > The special priority is defined in the net/wireless/ dir of the kernel, > and does not make special mention of mac80211 v/s other logic. > > From what I can tell, there is no other way to specify QoS when > sending on packet-sockets. The skb->priority never hits the wire > directly, so either something should be using it properly, or it > will just be ignored and we are no worse off than before. If > some driver is actually conflicting with the documentation, then > we just need to fix it. That code isn't really documentation though :) johannes
On 01/07/2014 06:59 AM, Johannes Berg wrote: > On Mon, 2013-09-23 at 09:04 -0700, Ben Greear wrote: >> On 09/23/2013 01:08 AM, Jouni Malinen wrote: >>> On Mon, Sep 09, 2013 at 11:49:04AM -0700, greearb@candelatech.com wrote: >>>> Without this patch, wpa_supplicant EAPOL packets (at least) >>>> are sent on normal best-effort TX queue. I believe they >>>> should be on the VO high-priority queue instead. >>> >>>> diff --git a/src/l2_packet/l2_packet_linux.c b/src/l2_packet/l2_packet_linux.c >>> >>>> @@ -97,6 +97,7 @@ struct l2_packet_data * l2_packet_init( >>>> + /* Use high-priority queue for management packets >>>> + * http://wireless.kernel.org/en/developers/Documentation/mac80211/queues >>>> + */ >>>> + if (setsockopt(l2->fd, SOL_SOCKET, >>>> + SO_PRIORITY, (char*)&val, sizeof(val)) < 0) { >>> >>> This seems to be assuming that mac80211 is used always. That is not >>> really the case and I guess this SO_PRIORITY change to a special >>> mac80211-specific value could result in undesired results with >>> non-mac80211 drivers. >> >> The special priority is defined in the net/wireless/ dir of the kernel, >> and does not make special mention of mac80211 v/s other logic. >> >> From what I can tell, there is no other way to specify QoS when >> sending on packet-sockets. The skb->priority never hits the wire >> directly, so either something should be using it properly, or it >> will just be ignored and we are no worse off than before. If >> some driver is actually conflicting with the documentation, then >> we just need to fix it. > > That code isn't really documentation though :) This page mentions it as well, about half way down. http://wireless.kernel.org/en/developers/Documentation/mac80211/queues As is, unless the NIC has firmware that inspects packets and twiddles the queue selection based on that, some management packets do not go out in the VO queue. With my patch, it (ath9k) seems to work properly. If someone has a suggestion for how to do this better, please let me know. Thanks, Ben
On Tue, 2014-01-07 at 10:12 -0800, Ben Greear wrote: > > That code isn't really documentation though :) > > This page mentions it as well, about half way down. > > http://wireless.kernel.org/en/developers/Documentation/mac80211/queues Which again is mac80211 documentation. > As is, unless the NIC has firmware that inspects packets and > twiddles the queue selection based on that, some management packets do not go > out in the VO queue. > > With my patch, it (ath9k) seems to work properly. Sure, but you're ignoring that there are drivers that aren't mac80211 based, nor forced to interpret the priority this way. At least not unless you go to the trouble of defining it as such in the kernel. Now, I'm not necessarily saying this would be a bad thing, but as it stands right now I'm not convinced that this can be done. johannes
On 01/15/2014 06:06 AM, Johannes Berg wrote: > On Tue, 2014-01-07 at 10:12 -0800, Ben Greear wrote: > >>> That code isn't really documentation though :) >> >> This page mentions it as well, about half way down. >> >> http://wireless.kernel.org/en/developers/Documentation/mac80211/queues > > Which again is mac80211 documentation. > >> As is, unless the NIC has firmware that inspects packets and >> twiddles the queue selection based on that, some management packets do not go >> out in the VO queue. >> >> With my patch, it (ath9k) seems to work properly. > > Sure, but you're ignoring that there are drivers that aren't mac80211 > based, nor forced to interpret the priority this way. At least not > unless you go to the trouble of defining it as such in the kernel. > > Now, I'm not necessarily saying this would be a bad thing, but as it > stands right now I'm not convinced that this can be done. I'm not trying to force a driver to do anything, but I would definitely like to suggest a driver give the management packets high priority, and for drivers I can use and modify, I will try to make sure they work properly. Other driver writers can do as they wish. If anyone can proclaim an API, then you can. If you have another API in mind, please detail it. If no one cares, then I can just carry the hostap patch in my own tree and move on to other things. Thanks, Ben > > johannes >
On Wed, 2014-01-15 at 09:18 -0800, Ben Greear wrote: > > Now, I'm not necessarily saying this would be a bad thing, but as it > > stands right now I'm not convinced that this can be done. > > I'm not trying to force a driver to do anything, but I would definitely > like to suggest a driver give the management packets high priority, > and for drivers I can use and modify, I will try to make sure they work > properly. Other driver writers can do as they wish. Sure, that's all well, I guess I'm just a bit worried that other drivers might interpret those priority values of 256+ as different specials, and the wrong thing would happen if this change was done generically in wpa_s. > If anyone can proclaim an API, then you can. If you have another API in mind, > please detail it. If no one cares, then I can just carry the hostap patch in > my own tree and move on to other things. The obvious other alternative would be to just do this in the kernel, assigning high priority to any packets that have tx->sdata->control_port_protocol == tx->skb->protocol (speaking in mac80211 language) - a number of drivers already do some special things via IEEE80211_TX_CTRL_PORT_CTRL_PROTO. I'm not entirely sure that's a better solution overall, but in terms of driver compatibility it would certainly be less risky. johannes
On 01/31/2014 05:55 AM, Johannes Berg wrote: > On Wed, 2014-01-15 at 09:18 -0800, Ben Greear wrote: > >>> Now, I'm not necessarily saying this would be a bad thing, but as it >>> stands right now I'm not convinced that this can be done. >> >> I'm not trying to force a driver to do anything, but I would definitely >> like to suggest a driver give the management packets high priority, >> and for drivers I can use and modify, I will try to make sure they work >> properly. Other driver writers can do as they wish. > > Sure, that's all well, I guess I'm just a bit worried that other drivers > might interpret those priority values of 256+ as different specials, and > the wrong thing would happen if this change was done generically in > wpa_s. Well, how many drivers are there, anyway? Maybe a simple grep would show any conflicts. I'll go poke around this when I get a chance. > >> If anyone can proclaim an API, then you can. If you have another API in mind, >> please detail it. If no one cares, then I can just carry the hostap patch in >> my own tree and move on to other things. > > The obvious other alternative would be to just do this in the kernel, > assigning high priority to any packets that have > tx->sdata->control_port_protocol == tx->skb->protocol (speaking in > mac80211 language) - a number of drivers already do some special things > via IEEE80211_TX_CTRL_PORT_CTRL_PROTO. > > I'm not entirely sure that's a better solution overall, but in terms of > driver compatibility it would certainly be less risky. In the past, there was a big push to move/keep 'policy' out of the kernel and into user-space. You would be doing the opposite. Your suggestion, unless backported, would not fix older kernels either. But, I don't care that much either way, so whatever you decide is fine with me. Thanks, Ben
On Fri, 2014-01-31 at 09:23 -0800, Ben Greear wrote: > Well, how many drivers are there, anyway? Maybe a simple grep would show > any conflicts. I'll go poke around this when I get a chance. I'd be more concerned about those drivers you *don't* know about, all those proprietary or semi-open drivers that use cfg80211/nl80211. Personally though, I'm not concerned, I know of no such driver that's worth worrying about, but Jouni might disagree :) > > The obvious other alternative would be to just do this in the kernel, > > assigning high priority to any packets that have > > tx->sdata->control_port_protocol == tx->skb->protocol (speaking in > > mac80211 language) - a number of drivers already do some special things > > via IEEE80211_TX_CTRL_PORT_CTRL_PROTO. > > > > I'm not entirely sure that's a better solution overall, but in terms of > > driver compatibility it would certainly be less risky. > > In the past, there was a big push to move/keep 'policy' out of the kernel > and into user-space. You would be doing the opposite. > > Your suggestion, unless backported, would not fix older kernels > either. Yeah, I'm not sure that's better. But I'm not sure relying on a special/magic API is better either? It's a grey area for sure ... johannes
On Fri, Jan 31, 2014 at 02:55:17PM +0100, Johannes Berg wrote: > The obvious other alternative would be to just do this in the kernel, > assigning high priority to any packets that have > tx->sdata->control_port_protocol == tx->skb->protocol (speaking in > mac80211 language) - a number of drivers already do some special things > via IEEE80211_TX_CTRL_PORT_CTRL_PROTO. > > I'm not entirely sure that's a better solution overall, but in terms of > driver compatibility it would certainly be less risky. With commit 1bf4bbb4024dcdab5e57634dd8ae1072d42a53ac ('mac80211: send control port protocol frames to the VO queue') now in wireless-testing.git and tagged for stable, I'm planning on dropping these wpa_supplicant/hostapd changes that use setsockopt() with the special SO_PRIORITY values.
On 02/25/2014 09:16 AM, Jouni Malinen wrote: > On Fri, Jan 31, 2014 at 02:55:17PM +0100, Johannes Berg wrote: >> The obvious other alternative would be to just do this in the kernel, >> assigning high priority to any packets that have >> tx->sdata->control_port_protocol == tx->skb->protocol (speaking in >> mac80211 language) - a number of drivers already do some special things >> via IEEE80211_TX_CTRL_PORT_CTRL_PROTO. >> >> I'm not entirely sure that's a better solution overall, but in terms of >> driver compatibility it would certainly be less risky. > > With commit 1bf4bbb4024dcdab5e57634dd8ae1072d42a53ac ('mac80211: send > control port protocol frames to the VO queue') now in > wireless-testing.git and tagged for stable, I'm planning on dropping > these wpa_supplicant/hostapd changes that use setsockopt() with the > special SO_PRIORITY values. Ok, that sounds fine to me. Anyone needing that functionality on older or non stable kernels can just apply the supplicant patch locally. Thanks, Ben
On 25 February 2014 19:39, Ben Greear <greearb@candelatech.com> wrote: > On 02/25/2014 09:16 AM, Jouni Malinen wrote: >> On Fri, Jan 31, 2014 at 02:55:17PM +0100, Johannes Berg wrote: >>> The obvious other alternative would be to just do this in the kernel, >>> assigning high priority to any packets that have >>> tx->sdata->control_port_protocol == tx->skb->protocol (speaking in >>> mac80211 language) - a number of drivers already do some special things >>> via IEEE80211_TX_CTRL_PORT_CTRL_PROTO. >>> >>> I'm not entirely sure that's a better solution overall, but in terms of >>> driver compatibility it would certainly be less risky. >> >> With commit 1bf4bbb4024dcdab5e57634dd8ae1072d42a53ac ('mac80211: send >> control port protocol frames to the VO queue') now in >> wireless-testing.git and tagged for stable, I'm planning on dropping >> these wpa_supplicant/hostapd changes that use setsockopt() with the >> special SO_PRIORITY values. > Regarding this mac80211 patch I am just curious what in such case AP (UAPSD) <--> STA (AC: VO - default we have in mac80211) STA go to PS AP send EAP (VO) packet - we will not set PVB STA will not wake-up ... I am not sure about real word here - if vendors/mac80211 allow STA go to PS during EAP exchange ... BR Janusz
On 26 February 2014 07:45, Janusz Dziedzic <janusz.dziedzic@tieto.com> wrote: > On 25 February 2014 19:39, Ben Greear <greearb@candelatech.com> wrote: >> On 02/25/2014 09:16 AM, Jouni Malinen wrote: >>> On Fri, Jan 31, 2014 at 02:55:17PM +0100, Johannes Berg wrote: >>>> The obvious other alternative would be to just do this in the kernel, >>>> assigning high priority to any packets that have >>>> tx->sdata->control_port_protocol == tx->skb->protocol (speaking in >>>> mac80211 language) - a number of drivers already do some special things >>>> via IEEE80211_TX_CTRL_PORT_CTRL_PROTO. >>>> >>>> I'm not entirely sure that's a better solution overall, but in terms of >>>> driver compatibility it would certainly be less risky. >>> >>> With commit 1bf4bbb4024dcdab5e57634dd8ae1072d42a53ac ('mac80211: send >>> control port protocol frames to the VO queue') now in >>> wireless-testing.git and tagged for stable, I'm planning on dropping >>> these wpa_supplicant/hostapd changes that use setsockopt() with the >>> special SO_PRIORITY values. >> > Regarding this mac80211 patch I am just curious what in such case > > AP (UAPSD) <--> STA (AC: VO - default we have in mac80211) > STA go to PS > AP send EAP (VO) packet - we will not set PVB > STA will not wake-up ... > > I am not sure about real word here - if vendors/mac80211 allow STA go > to PS during EAP exchange ... > Adding Felix. Did we introduce a possible bug in case of AP UAPSD? BR Janusz
On Mon, 2014-03-17 at 11:34 +0100, Janusz Dziedzic wrote: > > Regarding this mac80211 patch I am just curious what in such case > > > > AP (UAPSD) <--> STA (AC: VO - default we have in mac80211) > > STA go to PS > > AP send EAP (VO) packet - we will not set PVB > > STA will not wake-up ... > > > > I am not sure about real word here - if vendors/mac80211 allow STA go > > to PS during EAP exchange ... > > > > Adding Felix. > Did we introduce a possible bug in case of AP UAPSD? I don't see how any of this is a bug - if the client wants uAPSD then it better be prepared to handle VO packets coming to it in some way? johannes
On 17 March 2014 11:43, Johannes Berg <johannes@sipsolutions.net> wrote: > On Mon, 2014-03-17 at 11:34 +0100, Janusz Dziedzic wrote: > >> > Regarding this mac80211 patch I am just curious what in such case >> > >> > AP (UAPSD) <--> STA (AC: VO - default we have in mac80211) >> > STA go to PS >> > AP send EAP (VO) packet - we will not set PVB >> > STA will not wake-up ... >> > >> > I am not sure about real word here - if vendors/mac80211 allow STA go >> > to PS during EAP exchange ... >> > >> >> Adding Felix. >> Did we introduce a possible bug in case of AP UAPSD? > > I don't see how any of this is a bug - if the client wants uAPSD then it > better be prepared to handle VO packets coming to it in some way? > You mean HW that support UAPSD in STA mode: a) "in some" way send trigger frames when goes to PS and expect EAP frame b) should't go to PS during EAP exchange Do you know how MVM handle this? I am asking because mac80211 STA request VO by default (if UAPSD supported by HW and AP report UAPSD supported) and now we decided to send EAP also as a VO traffic from AP. BR Janusz
On Tue, 2014-03-18 at 07:10 +0100, Janusz Dziedzic wrote: > On 17 March 2014 11:43, Johannes Berg <johannes@sipsolutions.net> wrote: > > On Mon, 2014-03-17 at 11:34 +0100, Janusz Dziedzic wrote: > > > >> > Regarding this mac80211 patch I am just curious what in such case > >> > > >> > AP (UAPSD) <--> STA (AC: VO - default we have in mac80211) > >> > STA go to PS > >> > AP send EAP (VO) packet - we will not set PVB > >> > STA will not wake-up ... > >> > > >> > I am not sure about real word here - if vendors/mac80211 allow STA go > >> > to PS during EAP exchange ... > >> > > >> > >> Adding Felix. > >> Did we introduce a possible bug in case of AP UAPSD? > > > > I don't see how any of this is a bug - if the client wants uAPSD then it > > better be prepared to handle VO packets coming to it in some way? > > > You mean HW that support UAPSD in STA mode: > a) "in some" way send trigger frames when goes to PS and expect EAP frame > b) should't go to PS during EAP exchange > > Do you know how MVM handle this? I think we only go to PS after being authorized, but not sure. That's not the only case though since you can also get EAP frames for GTK rekeying. johannes
diff --git a/src/l2_packet/l2_packet_linux.c b/src/l2_packet/l2_packet_linux.c index 1419830..cf2a7cc 100644 --- a/src/l2_packet/l2_packet_linux.c +++ b/src/l2_packet/l2_packet_linux.c @@ -97,6 +97,7 @@ struct l2_packet_data * l2_packet_init( struct l2_packet_data *l2; struct ifreq ifr; struct sockaddr_ll ll; + int val = (256 + 7); l2 = os_zalloc(sizeof(struct l2_packet_data)); if (l2 == NULL) @@ -146,6 +147,18 @@ struct l2_packet_data * l2_packet_init( } os_memcpy(l2->own_addr, ifr.ifr_hwaddr.sa_data, ETH_ALEN); + + /* Use high-priority queue for management packets + * http://wireless.kernel.org/en/developers/Documentation/mac80211/queues + */ + if (setsockopt(l2->fd, SOL_SOCKET, + SO_PRIORITY, (char*)&val, sizeof(val)) < 0) { + /* Carry on...this is not fatal. */ + wpa_printf(MSG_DEBUG, + "l2_packet: sock priority sockopt (%i) failed\n", + val); + } + eloop_register_read_sock(l2->fd, l2_packet_receive, l2, NULL); return l2;