| Message ID | 20231211085121.8a1c7d1f0034.I50aed38be78ae9aea052938e2cb6b5800010ecd4@changeid |
|---|---|
| State | New |
| Headers | show |
| Series | cfg80211/mac80211 patches from our internal tree 2023-12-10 | expand |
> > The default DSCP-to-UP mapping method defined in RFC8325 > applied to packets marked per recommendations in RFC4594 and > destined to 802.11 WLAN clients will yield a number of inconsistent > QoS mappings. > > To handle this, modify the mapping of specific DSCP values for > which the default mapping will create inconsistencies, based on > the recommendations in section 4 in RFC8325. > This breaks ap_qosmap_default_acm ap_qosmap_default test cases. How should we handle that? johannes
Hi, > -----Original Message----- > From: Johannes Berg <johannes@sipsolutions.net> > Sent: Tuesday, December 12, 2023 11:40 > To: Korenblit, Miriam Rachel <miriam.rachel.korenblit@intel.com> > Cc: linux-wireless@vger.kernel.org; Peer, Ilan <ilan.peer@intel.com>; > Greenman, Gregory <gregory.greenman@intel.com>; Jouni Malinen <j@w1.fi> > Subject: Re: [PATCH 06/14] wifi: cfg80211: Update the default DSCP-to-UP > mapping > > > > > The default DSCP-to-UP mapping method defined in RFC8325 applied to > > packets marked per recommendations in RFC4594 and destined to 802.11 > > WLAN clients will yield a number of inconsistent QoS mappings. > > > > To handle this, modify the mapping of specific DSCP values for which > > the default mapping will create inconsistencies, based on the > > recommendations in section 4 in RFC8325. > > > > This breaks ap_qosmap_default_acm ap_qosmap_default test cases. > > How should we handle that? Corresponding change for hostap: https://patchwork.ozlabs.org/project/hostap/patch/20231213113735.289408-1-andrei.otcheretianski@intel.com/. Regards, Ilan.
On Mon, Dec 11, 2023 at 09:05:24AM +0200, Miri Korenblit wrote: > The default DSCP-to-UP mapping method defined in RFC8325 > applied to packets marked per recommendations in RFC4594 and > destined to 802.11 WLAN clients will yield a number of inconsistent > QoS mappings. > > To handle this, modify the mapping of specific DSCP values for > which the default mapping will create inconsistencies, based on > the recommendations in section 4 in RFC8325. Could this commit message give some more justification for why these exact specifications are the best source of this information? For example, should this also reference the Wi-Fi QoS Management specification? > diff --git a/net/wireless/util.c b/net/wireless/util.c > @@ -980,7 +980,53 @@ unsigned int cfg80211_classify8021d(struct sk_buff *skb, > } > } > > + /* The default mapping as defined in RFC8325 */ > ret = dscp >> 5; Could you point to the exact location in RFC 8325 that has that as the default mapping? Figure 1 has this note: "Note: All unused codepoints are RECOMMENDED to be mapped to UP 0". Section 2.4 does describe what many APs do, but that section is called "Default UP-to-DSCP Mappings and Conflicts" which does not sound like a recommended default mapping in RFC 8325.. > + /* Handle specific DSCP values for which the default mapping doesn't > + * adhere to the intended usage of the DSCP value. See section 4 in > + * RFC8325. > + */ > + switch (dscp >> 2) { What about "Standard: DF" (0 --> 0) and "Low-Priority Data: CS1" (8 -> 1)? Are those omitted here because the "dscp >> 5" happens to have same value? It would seem to be good to have at least a comment here pointing that out in particular taken into account that comment above about the status of that "default mapping" and how it relates to RFC 8325. > + case 10: > + case 12: > + case 14: > + /* High throughput data: AF11, AF12, AF13 */ > + ret = 0; > + break; > + case 16: > + /* Operations, Administration, and Maintenance and Provisioning: > + * CS2 > + */ > + ret = 0; > + break; > + case 18: > + case 20: > + case 22: > + /* Low latency data: AF21, AF22, AF23 */ > + ret = 3; > + break; > + case 24: > + /* Broadcasting video: CS23 */ > + ret = 4; > + break; Typo.. Should be "CS3" instead of "CS23". What about "Multimedia Streaming: AF31, AF32, AF33"? Shouldn't those values 26, 28, 30 be mapped to 4? If not, it would be good to add a comment explaining why that is not here while it is included in RFC 8325 Figure 1. What about "Real-Time Interactive: CS4"? Shouldn't that value 32 be mapped to 4? If not, it would be good to add a comment explaining why that is not here while it is included in RFC 8325 Figure 1. What about "Multimedia Conferencing: AF41, AF42, AF43"? Shouldn't those values 34, 36, 38 be mapped to 4? If not, it would be good to add a comment explaining why that is not here while it is included in RFC 8325 Figure 1. > + case 40: > + /* Signaling: CS5 */ > + ret = 5; > + break; > + case 44: > + /* Voice Admit */ > + ret = 6; > + break; To be consistent, that comment should be "VOICE-ADMIT: VA". > + case 46: > + /* Telephony traffic: EF */ > + ret = 6; > + break; > + case 48: > + /* Network Control Traffic: CS6 */ > + ret = 7; > + break; > + } This does not include "Network Control: CS7". Is there a reason for not covering that case 56 now? I know it is "reserved for future use", but RFC 8325 does provide mapping for it as well.
Hi, > -----Original Message----- > From: Jouni Malinen <j@w1.fi> > Sent: Sunday, December 17, 2023 11:42 > To: Korenblit, Miriam Rachel <miriam.rachel.korenblit@intel.com> > Cc: johannes@sipsolutions.net; linux-wireless@vger.kernel.org; Peer, Ilan > <ilan.peer@intel.com>; Greenman, Gregory <gregory.greenman@intel.com>; > Berg, Johannes <johannes.berg@intel.com> > Subject: Re: [PATCH 06/14] wifi: cfg80211: Update the default DSCP-to-UP > mapping > > On Mon, Dec 11, 2023 at 09:05:24AM +0200, Miri Korenblit wrote: > > The default DSCP-to-UP mapping method defined in RFC8325 applied to > > packets marked per recommendations in RFC4594 and destined to 802.11 > > WLAN clients will yield a number of inconsistent QoS mappings. > > > > To handle this, modify the mapping of specific DSCP values for which > > the default mapping will create inconsistencies, based on the > > recommendations in section 4 in RFC8325. > > Could this commit message give some more justification for why these exact > specifications are the best source of this information? For example, should this > also reference the Wi-Fi QoS Management specification? > I was using these specifications since they were mentioned in the Wi-Fi QoS Management specification and because RFC8325 was specifically mentioned in the WFA test plan specifications. I'll update the commit message. > > diff --git a/net/wireless/util.c b/net/wireless/util.c @@ -980,7 > > +980,53 @@ unsigned int cfg80211_classify8021d(struct sk_buff *skb, > > } > > } > > > > + /* The default mapping as defined in RFC8325 */ > > ret = dscp >> 5; > > Could you point to the exact location in RFC 8325 that has that as the default > mapping? Figure 1 has this note: "Note: All unused codepoints are > RECOMMENDED to be mapped to UP 0". > > Section 2.4 does describe what many APs do, but that section is called > "Default UP-to-DSCP Mappings and Conflicts" which does not sound like a > recommended default mapping in RFC 8325.. > The term "Default DSCP-to-UP" is defined in section 2.3 in RFC8325. As noted there, this is not a specification definition, but more a term to describe what is a "common practice in the networking industry". > > + /* Handle specific DSCP values for which the default mapping doesn't > > + * adhere to the intended usage of the DSCP value. See section 4 in > > + * RFC8325. > > + */ > > + switch (dscp >> 2) { > > What about "Standard: DF" (0 --> 0) and "Low-Priority Data: CS1" (8 -> 1)? > Are those omitted here because the "dscp >> 5" happens to have same value? > It would seem to be good to have at least a comment here pointing that out in > particular taken into account that comment above about the status of that > "default mapping" and how it relates to RFC 8325. > Yes. I thought my comment above was clear about it, but I'll rephrase it to make It clearer. > > + case 10: > > + case 12: > > + case 14: > > + /* High throughput data: AF11, AF12, AF13 */ > > + ret = 0; > > + break; > > + case 16: > > + /* Operations, Administration, and Maintenance and > Provisioning: > > + * CS2 > > + */ > > + ret = 0; > > + break; > > + case 18: > > + case 20: > > + case 22: > > + /* Low latency data: AF21, AF22, AF23 */ > > + ret = 3; > > + break; > > + case 24: > > + /* Broadcasting video: CS23 */ > > + ret = 4; > > + break; > > Typo.. Should be "CS3" instead of "CS23". > Fixed. > What about "Multimedia Streaming: AF31, AF32, AF33"? Shouldn't those > values 26, 28, 30 be mapped to 4? If not, it would be good to add a comment > explaining why that is not here while it is included in RFC 8325 Figure 1. > The default mapping maps AF31, AF32 and AF33 to 4 which is the recommended mapping. I'll add a comment. > What about "Real-Time Interactive: CS4"? Shouldn't that value 32 be mapped > to 4? If not, it would be good to add a comment explaining why that is not > here while it is included in RFC 8325 Figure 1. > Same here. The default mapping is used. I'll add a comment. > What about "Multimedia Conferencing: AF41, AF42, AF43"? Shouldn't those > values 34, 36, 38 be mapped to 4? If not, it would be good to add a comment > explaining why that is not here while it is included in RFC 8325 Figure 1. > Same. > > + case 40: > > + /* Signaling: CS5 */ > > + ret = 5; > > + break; > > + case 44: > > + /* Voice Admit */ > > + ret = 6; > > + break; > > To be consistent, that comment should be "VOICE-ADMIT: VA". > Fixed. > > + case 46: > > + /* Telephony traffic: EF */ > > + ret = 6; > > + break; > > + case 48: > > + /* Network Control Traffic: CS6 */ > > + ret = 7; > > + break; > > + } > > This does not include "Network Control: CS7". Is there a reason for not > covering that case 56 now? I know it is "reserved for future use", but RFC > 8325 does provide mapping for it as well. > Same. Regards, Ilan.
diff --git a/net/wireless/util.c b/net/wireless/util.c index 626b858b4b35..dd93e1950921 100644 --- a/net/wireless/util.c +++ b/net/wireless/util.c @@ -980,7 +980,53 @@ unsigned int cfg80211_classify8021d(struct sk_buff *skb, } } + /* The default mapping as defined in RFC8325 */ ret = dscp >> 5; + + /* Handle specific DSCP values for which the default mapping doesn't + * adhere to the intended usage of the DSCP value. See section 4 in + * RFC8325. + */ + switch (dscp >> 2) { + case 10: + case 12: + case 14: + /* High throughput data: AF11, AF12, AF13 */ + ret = 0; + break; + case 16: + /* Operations, Administration, and Maintenance and Provisioning: + * CS2 + */ + ret = 0; + break; + case 18: + case 20: + case 22: + /* Low latency data: AF21, AF22, AF23 */ + ret = 3; + break; + case 24: + /* Broadcasting video: CS23 */ + ret = 4; + break; + case 40: + /* Signaling: CS5 */ + ret = 5; + break; + case 44: + /* Voice Admit */ + ret = 6; + break; + case 46: + /* Telephony traffic: EF */ + ret = 6; + break; + case 48: + /* Network Control Traffic: CS6 */ + ret = 7; + break; + } out: return array_index_nospec(ret, IEEE80211_NUM_TIDS); }