From: cedric voncken <[email protected]>
If the VLAN tci is set in skb->vlan_tci use the priority field to determine the WMM priority.
V2 modifications :
Fix indentation
Use symbolic constant
include the header linux/if_vlan.h
Signed-off-by: cedric Voncken <[email protected]>
---
net/wireless/util.c | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/net/wireless/util.c b/net/wireless/util.c
index 74458b7..13937db 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -10,6 +10,7 @@
#include <net/cfg80211.h>
#include <net/ip.h>
#include <net/dsfield.h>
+#include <linux/if_vlan.h>
#include "core.h"
#include "rdev-ops.h"
@@ -685,6 +686,7 @@ EXPORT_SYMBOL(ieee80211_amsdu_to_8023s);
unsigned int cfg80211_classify8021d(struct sk_buff *skb)
{
unsigned int dscp;
+ unsigned char vlan_priority;
/* skb->priority values from 256->263 are magic values to
* directly indicate a specific 802.1d priority. This is used
@@ -694,6 +696,10 @@ unsigned int cfg80211_classify8021d(struct sk_buff *skb)
if (skb->priority >= 256 && skb->priority <= 263)
return skb->priority - 256;
+ vlan_priority = (skb->vlan_tci & VLAN_PRIO_MASK) >> VLAN_PRIO_SHIFT;
+ if (vlan_priority > 0)
+ return vlan_priority;
+
switch (skb->protocol) {
case htons(ETH_P_IP):
dscp = ipv4_get_dsfield(ip_hdr(skb)) & 0xfc;
--
1.7.2.5
> >+ vlan_priority = (skb->vlan_tci & VLAN_PRIO_MASK) >> VLAN_PRIO_SHIFT;
> >+ if (vlan_priority > 0)
>> + return vlan_priority;
>Is this really correct? For once, checking vlan_priority for non-zero seems weird since that ought to be a valid value -- is there no other way to determine that the value is valid?
The vlan Tag contain three bit for priority. The value 0 indicate no priority (on this case the VLAN tag contain only VID). The vlan_tci field is set to zero if the frame do not contain the vlan tag. So if we have not a vlan tag or no priority in VLAN tag the priority value is always 0.
>Also, this gives you a 2-bit value, while the return value should be a 3-bit value, maybe you need to shift this up by one to spread into the correct buckets?
Sorry but I don't understand. The vlan_tci field it is a __u16 value (defined in include/linux/skbuff.h), the VLAN_PRIO_MASK is set to 0xE000 and VLAN_PRIO_SHIFT is set to 13 (defined in include/linux/if_vlan.h), the vlan_priority is an unsigned char. For me the vlan_priority contain a 3-bit value (0xE000 >>13 = 0x0003), why 2 ?
If you are agree with me, I'll resend a V3 patch with a correct subject.
Cedric
>johannes
> + vlan_priority = (skb->vlan_tci & VLAN_PRIO_MASK) >> VLAN_PRIO_SHIFT;
> + if (vlan_priority > 0)
> + return vlan_priority;
Is this really correct? For once, checking vlan_priority for non-zero
seems weird since that ought to be a valid value -- is there no other
way to determine that the value is valid?
Also, this gives you a 2-bit value, while the return value should be a
3-bit value, maybe you need to shift this up by one to spread into the
correct buckets?
johannes
On Thu, 2013-07-04 at 09:18 +0200, voncken wrote:
> If the VLAN tci is set in skb->vlan_tci use the priority field to determine
> the WMM priority.
Pinging me and resending your patch isn't going to make it more likely
that I apply it. We're in the merge window, so naturally patch
application is stalled.
johannes
> > The vlan Tag contain three bit for priority. The value 0 indicate no
> > priority (on this case the VLAN tag contain only VID). The vlan_tci
> > field is set to zero if the frame do not contain the vlan tag. So if
> > we have not a vlan tag or no priority in VLAN tag the priority value
> > is always 0.
> Yes but don't we know that the vlan_tci field is valid?
> I don't think you're correct in that 0 means "no priority present", it actually means "best effort" as far as I can tell. Ignoring the VLAN tag when the field is 0 would mean we could use a higher priority from the contents of the frame, which would not be desired?
I can add a test with the macro vlan_tx_tag_present() to verify if the vlan_tci field is valid.
I test the value 0 to skip the VLAN priority and use the dscp priority in this case. The priority 0 in VLAN tag is often use to turn off the QOS, because not bit is allowed for it.
For me is it correct. Nevertheless, if you prefer, I can test only the vlan_tci validity and in this case always use the VLAN priority.
> > Sorry but I don't understand. The vlan_tci field it is a __u16 value
> > (defined in include/linux/skbuff.h), the VLAN_PRIO_MASK is set to
> > 0xE000 and VLAN_PRIO_SHIFT is set to 13 (defined in
> > include/linux/if_vlan.h), the vlan_priority is an unsigned char. For
> > me the vlan_priority contain a 3-bit value (0xE000 >>13 = 0x0003), why
> > 2 ?
> Umm? No? Think again about what hweight(0x0003) is.
Sorry I made a mistake 0xE000 >>13 = 0x0007 and not 0x0003, and 7 is a 3 bits value.
Cedric
Subject line should have a "wireless: " (or "cfg80211: ") prefix.
> If the VLAN tci is set in skb->vlan_tci use the priority field to determine the WMM priority.
>
> V2 modifications :
> Fix indentation
> Use symbolic constant
> include the header linux/if_vlan.h
This doesn't belong into the changelog, it should be after --- with the
diffstat.
johannes
Sorry but I have some problems with my mail server, and actually, I have a lot of mail not sent. So I was not sure you received my message.
I waiting the end of merge windows
Cedric
-----Message d'origine-----
De : Johannes Berg [mailto:[email protected]]
Envoyé : jeudi 4 juillet 2013 09:47
À : voncken
Cc : [email protected]
Objet : Re: [PATCH V2] vlan priority handling in WMM
On Thu, 2013-07-04 at 09:18 +0200, voncken wrote:
> If the VLAN tci is set in skb->vlan_tci use the priority field to
> determine the WMM priority.
Pinging me and resending your patch isn't going to make it more likely that I apply it. We're in the merge window, so naturally patch application is stalled.
johannes
If the VLAN tci is set in skb->vlan_tci use the priority field to determine
the WMM priority.
V2 modifications :
Fix indentation
Use symbolic constant
include the header linux/if_vlan.h
Signed-off-by: cedric Voncken <[email protected]>
---
net/wireless/util.c | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/net/wireless/util.c b/net/wireless/util.c index
74458b7..13937db 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -10,6 +10,7 @@
#include <net/cfg80211.h>
#include <net/ip.h>
#include <net/dsfield.h>
+#include <linux/if_vlan.h>
#include "core.h"
#include "rdev-ops.h"
@@ -685,6 +686,7 @@ EXPORT_SYMBOL(ieee80211_amsdu_to_8023s);
unsigned int cfg80211_classify8021d(struct sk_buff *skb) {
unsigned int dscp;
+ unsigned char vlan_priority;
/* skb->priority values from 256->263 are magic values to
* directly indicate a specific 802.1d priority. This is used @@
-694,6 +696,10 @@ unsigned int cfg80211_classify8021d(struct sk_buff *skb)
if (skb->priority >= 256 && skb->priority <= 263)
return skb->priority - 256;
+ vlan_priority = (skb->vlan_tci & VLAN_PRIO_MASK) >> VLAN_PRIO_SHIFT;
+ if (vlan_priority > 0)
+ return vlan_priority;
+
switch (skb->protocol) {
case htons(ETH_P_IP):
dscp = ipv4_get_dsfield(ip_hdr(skb)) & 0xfc;
--
1.7.2.5
On Fri, 2013-07-05 at 16:39 +0200, voncken wrote:
> > >+ vlan_priority = (skb->vlan_tci & VLAN_PRIO_MASK) >>
> VLAN_PRIO_SHIFT;
> > >+ if (vlan_priority > 0)
> >> + return vlan_priority;
>
> > Is this really correct? For once, checking vlan_priority for
> non-zero seems
> > weird since that ought to be a valid value -- is there no other way
> to
> > determine that the value is valid?
> The vlan Tag contain three bit for priority. The value 0 indicate no
> priority (on this case the VLAN tag contain only VID). The vlan_tci
> field is set to zero if the frame do not contain the vlan tag. So if
> we have not a vlan tag or no priority in VLAN tag the priority value
> is always 0.
Yes but don't we know that the vlan_tci field is valid?
I don't think you're correct in that 0 means "no priority present", it
actually means "best effort" as far as I can tell. Ignoring the VLAN tag
when the field is 0 would mean we could use a higher priority from the
contents of the frame, which would not be desired?
> >Also, this gives you a 2-bit value, while the return value should be
> a 3-bit value, maybe you need to shift this up by one to spread into
> the correct buckets?
> Sorry but I don't understand. The vlan_tci field it is a __u16 value
> (defined in include/linux/skbuff.h), the VLAN_PRIO_MASK is set to
> 0xE000 and VLAN_PRIO_SHIFT is set to 13 (defined in
> include/linux/if_vlan.h), the vlan_priority is an unsigned char. For
> me the vlan_priority contain a 3-bit value (0xE000 >>13 = 0x0003), why
> 2 ?
Umm? No? Think again about what hweight(0x0003) is.
johannes
On Mon, 2013-07-08 at 12:39 +0200, voncken wrote:
> > > The vlan Tag contain three bit for priority. The value 0 indicate
> no
> > > priority (on this case the VLAN tag contain only VID). The
> vlan_tci
> > > field is set to zero if the frame do not contain the vlan tag. So
> if
> > > we have not a vlan tag or no priority in VLAN tag the priority
> value
> > > is always 0.
>
> > Yes but don't we know that the vlan_tci field is valid?
>
> > I don't think you're correct in that 0 means "no priority present",
> it actually means "best effort" as far as I can tell. Ignoring the
> VLAN tag when the field is 0 would mean we could use a higher priority
> from the contents of the frame, which would not be desired?
>
> I can add a test with the macro vlan_tx_tag_present() to verify if the
> vlan_tci field is valid.
> I test the value 0 to skip the VLAN priority and use the dscp priority
> in this case. The priority 0 in VLAN tag is often use to turn off the
> QOS, because not bit is allowed for it.
What do you mean by "is often used"? I don't see how that would be the
case? Are you saying routers commonly ignore the VLAN priority value if
it's 0? That would seem odd?
> For me is it correct. Nevertheless, if you prefer, I can test only the
> vlan_tci validity and in this case always use the VLAN priority.
I don't know! Since you don't seem to really know either, we should ask
somebody who knows, I think. Maybe you should Cc netdev with this
question on the patch or so?
> Sorry I made a mistake 0xE000 >>13 = 0x0007 and not 0x0003, and 7 is
> a 3 bits value.
Ah yes, I made the same mistake, sorry.
johannes
Any comments on this patch ?
Cedric
-----Message d'origine-----
De?: [email protected]
[mailto:[email protected]] De la part de
[email protected]
Envoy??: jeudi 27 juin 2013 09:52
??: [email protected]
Cc?: [email protected]; cedric voncken
Objet?: [PATCH V2] vlan priority handling in WMM
From: cedric voncken <[email protected]>
If the VLAN tci is set in skb->vlan_tci use the priority field to determine
the WMM priority.
V2 modifications :
Fix indentation
Use symbolic constant
include the header linux/if_vlan.h
Signed-off-by: cedric Voncken <[email protected]>
---
net/wireless/util.c | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/net/wireless/util.c b/net/wireless/util.c index
74458b7..13937db 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -10,6 +10,7 @@
#include <net/cfg80211.h>
#include <net/ip.h>
#include <net/dsfield.h>
+#include <linux/if_vlan.h>
#include "core.h"
#include "rdev-ops.h"
@@ -685,6 +686,7 @@ EXPORT_SYMBOL(ieee80211_amsdu_to_8023s);
unsigned int cfg80211_classify8021d(struct sk_buff *skb) {
unsigned int dscp;
+ unsigned char vlan_priority;
/* skb->priority values from 256->263 are magic values to
* directly indicate a specific 802.1d priority. This is used @@
-694,6 +696,10 @@ unsigned int cfg80211_classify8021d(struct sk_buff *skb)
if (skb->priority >= 256 && skb->priority <= 263)
return skb->priority - 256;
+ vlan_priority = (skb->vlan_tci & VLAN_PRIO_MASK) >> VLAN_PRIO_SHIFT;
+ if (vlan_priority > 0)
+ return vlan_priority;
+
switch (skb->protocol) {
case htons(ETH_P_IP):
dscp = ipv4_get_dsfield(ip_hdr(skb)) & 0xfc;
--
1.7.2.5