| Message ID | 20240526083112.447466-1-roid@nvidia.com |
|---|---|
| State | Accepted |
| Commit | 6280f5d04a8daad2ad7c5723da05e92b217877e2 |
| Delegated to: | Simon Horman |
| Headers | show |
| Series | [ovs-dev,1/1] netdev-offload-tc: Reserve lower tc prio for vlan ethertype. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On 5/26/24 10:31, Roi Dayan via dev wrote: > From: Maor Dickman <maord@nvidia.com> > > The cited commit reserved lower tc priorities for IP ethertypes in order > to give IP traffic higher priority than other management traffic. > In case of of vlan encap traffic, IP traffic will still get lower > priority. > > Fix it by also reserving low priority tc prio for vlan. > > Fixes: c230c7579c14 ("netdev-offload-tc: Reserve lower tc prios for ip ethertypes") > Signed-off-by: Maor Dickman <maord@nvidia.com> > Acked-by: Roi Dayan <roid@nvidia.com> > --- > lib/netdev-offload-tc.c | 2 ++ > lib/tc.h | 1 + > 2 files changed, 3 insertions(+) > > diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c > index 921d5231777e..3be1c08d24f6 100644 > --- a/lib/netdev-offload-tc.c > +++ b/lib/netdev-offload-tc.c > @@ -400,6 +400,8 @@ get_next_available_prio(ovs_be16 protocol) > return TC_RESERVED_PRIORITY_IPV4; > } else if (protocol == htons(ETH_P_IPV6)) { > return TC_RESERVED_PRIORITY_IPV6; > + } else if (protocol == htons(ETH_P_8021Q)) { Should 802.1ad traffic also get the priority? What about MPLS? Best regards, Ilya Maximets.
On 28/05/2024 20:12, Ilya Maximets wrote: > On 5/26/24 10:31, Roi Dayan via dev wrote: >> From: Maor Dickman <maord@nvidia.com> >> >> The cited commit reserved lower tc priorities for IP ethertypes in order >> to give IP traffic higher priority than other management traffic. >> In case of of vlan encap traffic, IP traffic will still get lower >> priority. >> >> Fix it by also reserving low priority tc prio for vlan. >> >> Fixes: c230c7579c14 ("netdev-offload-tc: Reserve lower tc prios for ip ethertypes") >> Signed-off-by: Maor Dickman <maord@nvidia.com> >> Acked-by: Roi Dayan <roid@nvidia.com> >> --- >> lib/netdev-offload-tc.c | 2 ++ >> lib/tc.h | 1 + >> 2 files changed, 3 insertions(+) >> >> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c >> index 921d5231777e..3be1c08d24f6 100644 >> --- a/lib/netdev-offload-tc.c >> +++ b/lib/netdev-offload-tc.c >> @@ -400,6 +400,8 @@ get_next_available_prio(ovs_be16 protocol) >> return TC_RESERVED_PRIORITY_IPV4; >> } else if (protocol == htons(ETH_P_IPV6)) { >> return TC_RESERVED_PRIORITY_IPV6; >> + } else if (protocol == htons(ETH_P_8021Q)) { > > Should 802.1ad traffic also get the priority? > What about MPLS? > > Best regards, Ilya Maximets. Hi Ilya, It is correct there could be more types that could benefit from a reserved prio but from what we saw in the field we didn't notice a real usage of 802.1ad or MPLS while 8021q was being used actively and we noticed the performance degradation and improvement after using the reserved prio. We think this patch can help many active openvswitch users and in the future if other types would be more common we could add those. Thanks, Roi
On Thu, May 30, 2024 at 09:31:06AM +0300, Roi Dayan via dev wrote: > > > On 28/05/2024 20:12, Ilya Maximets wrote: > > On 5/26/24 10:31, Roi Dayan via dev wrote: > >> From: Maor Dickman <maord@nvidia.com> > >> > >> The cited commit reserved lower tc priorities for IP ethertypes in order > >> to give IP traffic higher priority than other management traffic. > >> In case of of vlan encap traffic, IP traffic will still get lower > >> priority. > >> > >> Fix it by also reserving low priority tc prio for vlan. > >> > >> Fixes: c230c7579c14 ("netdev-offload-tc: Reserve lower tc prios for ip ethertypes") > >> Signed-off-by: Maor Dickman <maord@nvidia.com> > >> Acked-by: Roi Dayan <roid@nvidia.com> > >> --- > >> lib/netdev-offload-tc.c | 2 ++ > >> lib/tc.h | 1 + > >> 2 files changed, 3 insertions(+) > >> > >> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c > >> index 921d5231777e..3be1c08d24f6 100644 > >> --- a/lib/netdev-offload-tc.c > >> +++ b/lib/netdev-offload-tc.c > >> @@ -400,6 +400,8 @@ get_next_available_prio(ovs_be16 protocol) > >> return TC_RESERVED_PRIORITY_IPV4; > >> } else if (protocol == htons(ETH_P_IPV6)) { > >> return TC_RESERVED_PRIORITY_IPV6; > >> + } else if (protocol == htons(ETH_P_8021Q)) { > > > > Should 802.1ad traffic also get the priority? > > What about MPLS? > > > > Best regards, Ilya Maximets. > > Hi Ilya, > > It is correct there could be more types that could benefit from a reserved > prio but from what we saw in the field we didn't notice a real usage > of 802.1ad or MPLS while 8021q was being used actively and we noticed the > performance degradation and improvement after using the reserved prio. > We think this patch can help many active openvswitch users and in the future > if other types would be more common we could add those. Hi Roi and Ilya, FWIIW, I think it is reasonable to update 8021q up-front, i.e. accept this patch. But I also think that, as a follow-up, we should look into other Ether Types and see if they also need this treatment, rather than waiting for problems to materialise in the field. Roi, is this something your team can take on?
Hi Simon, I appreciate the review, Yes we will look in some of the other ether types and see if it's something we think is needed to prioritize as well. Thanks, Roi Sent from Nine<http://www.9folders.com/>
On Thu, Jun 06, 2024 at 10:54:45AM +0000, Roi Dayan wrote: > Hi Simon, > > I appreciate the review, Yes we will look in some of the other ether > types and see if it's something we think is needed to prioritize as well. Thanks Roi, Much appreciated. I've gone ahead and applied this patch to main. - netdev-offload-tc: Reserve lower tc prio for vlan ethertype. https://github.com/openvswitch/ovs/commit/6280f5d04a8d
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c index 921d5231777e..3be1c08d24f6 100644 --- a/lib/netdev-offload-tc.c +++ b/lib/netdev-offload-tc.c @@ -400,6 +400,8 @@ get_next_available_prio(ovs_be16 protocol) return TC_RESERVED_PRIORITY_IPV4; } else if (protocol == htons(ETH_P_IPV6)) { return TC_RESERVED_PRIORITY_IPV6; + } else if (protocol == htons(ETH_P_8021Q)) { + return TC_RESERVED_PRIORITY_VLAN; } } diff --git a/lib/tc.h b/lib/tc.h index fdbcf4b7cb25..8442c8d8b8cf 100644 --- a/lib/tc.h +++ b/lib/tc.h @@ -51,6 +51,7 @@ enum tc_flower_reserved_prio { TC_RESERVED_PRIORITY_POLICE, TC_RESERVED_PRIORITY_IPV4, TC_RESERVED_PRIORITY_IPV6, + TC_RESERVED_PRIORITY_VLAN, __TC_RESERVED_PRIORITY_MAX }; #define TC_RESERVED_PRIORITY_MAX (__TC_RESERVED_PRIORITY_MAX -1)