[ovs-dev] [PATCH v10] netdev-dpdk: add control plane protection support
Some control protocols are used to maintain link status between forwarding engines (e.g. LACP). When the system is not sized properly, the PMD threads may not be able to process all incoming traffic from the configured Rx queues. When a signaling packet of such protocols is dropped, it can cause link flapping, worsening the situation. Use the RTE flow API to redirect these protocols into a dedicated Rx queue. The assumption is made that the ratio between control protocol traffic and user data traffic is very low and thus this dedicated Rx queue will never get full. The RSS redirection table is re-programmed to only use the other Rx queues. The RSS table size is stored in the netdev_dpdk structure at port initialization to avoid requesting the information again when changing the port configuration. The additional Rx queue will be assigned a PMD core like any other Rx queue. Polling that extra queue may introduce increased latency and a slight performance penalty at the benefit of preventing link flapping. This feature must be enabled per port on specific protocols via the cp-protection option. This option takes a coma-separated list of protocol names. It is only supported on ethernet ports. This feature is experimental. If the user has already configured multiple Rx queues on the port, an additional one will be allocated for control plane packets. If the hardware cannot satisfy the requested number of requested Rx queues, the last Rx queue will be assigned for control plane. If only one Rx queue is available, the cp-protection feature will be disabled. If the hardware does not support the RTE flow matchers/actions, the feature will be disabled. It cannot be enabled when other_config:hw-offload=true as it may conflict with the offloaded RTE flows. Similarly, if hw-offload is enabled, cp-protection will be forcibly disabled on all ports. Example use: ovs-vsctl add-bond br-phy bond0 phy0 phy1 -- \ set interface phy0 type=dpdk options:dpdk-devargs=:ca:00.0 -- \ set interface phy0 options:cp-protection=lacp -- \ set interface phy1 type=dpdk options:dpdk-devargs=:ca:00.1 -- \ set interface phy1 options:cp-protection=lacp As a starting point, only one protocol is supported: LACP. Other protocols can be added in the future. NIC compatibility should be checked. To validate that this works as intended, I used a traffic generator to generate random traffic slightly above the machine capacity at line rate on a two ports bond interface. OVS is configured to receive traffic on two VLANs and pop/push them in a br-int bridge based on tags set on patch ports. +--+ | DUT | |++| || br-int || in_port=patch10,actions=mod_dl_src:$patch11,mod_dl_dst:$tgen1,output:patch11 |||| in_port=patch11,actions=mod_dl_src:$patch10,mod_dl_dst:$tgen0,output:patch10 || patch10patch11 || |+---|---|+| || | | |+---|---|+| || patch00patch01 || || tag:10tag:20 || |||| || br-phy || default flow, action=NORMAL |||| || bond0|| balance-slb, lacp=passive, lacp-time=fast ||phy0 phy1 || |+--|-|---+| +---|-|+ | | +---|-|+ | port0 port1 | balance L3/L4, lacp=active, lacp-time=fast | lag | mode trunk VLANs 10, 20 | | |switch| | | | vlan 10vlan 20 | mode access | port2 port3 | +-|--|-+ | | +-|--|-+ | tgen0 tgen1 | Random traffic that is properly balanced | | across the bond ports in both directions. | traffic generator | +--+ Without cp-protection, the bond0 links are randomly switching to "defaulted" when one of the LACP packets sent by the switch is dropped because the RX queues are full and the PMD threads did not process them fast enough. When that happens, all traffic must go through a single link which causes above line rate traffic to be dropped. ~# ovs-appctl lacp/show-stats bond0 bond0 statistics member: phy0: TX PDUs: 347246 RX PDUs: 14865 RX Bad PDUs: 0 RX Marker Request PDUs: 0 Link Expired: 168 Link Defaulted: 0 Carrier Status Changed: 0 member: phy1: TX PDUs: 347245 RX PDUs: 14919 RX Bad PDUs: 0 RX Marker Request PDUs: 0 Link Expired: 147 Link Defaulted: 1 Carrier Status Changed: 0 When cp-protection is enabled, no LACP packet is dropped and the bond links remain enabled at all times, maximizing the throughput. Neither the "Link Expired" nor the "Link Defaulted" counters are incremented anymore. This feature may be considered as "QoS". However, it does not work by limiting the rate of
Re: [ovs-dev] [PATCH v10] netdev-dpdk: add control plane protection support
Robin Jarry writes: > Some control protocols are used to maintain link status between > forwarding engines (e.g. LACP). When the system is not sized properly, > the PMD threads may not be able to process all incoming traffic from the > configured Rx queues. When a signaling packet of such protocols is > dropped, it can cause link flapping, worsening the situation. I think one issue I have with this is that the name is a bit misleading. Control plane, from OVS perspective, would be like OpenFlow communications. This is more like a traffic steering mechanism. Maybe it would help to call it something like "traffic-based-rps" or something like that (but not clear what would be best). It is really a way to steer specific traffic to a distinct rxq. WDYT? I didn't look much at the code this time - maybe Kevin has taken a look? > Use the RTE flow API to redirect these protocols into a dedicated Rx > queue. The assumption is made that the ratio between control protocol > traffic and user data traffic is very low and thus this dedicated Rx > queue will never get full. The RSS redirection table is re-programmed to > only use the other Rx queues. The RSS table size is stored in the > netdev_dpdk structure at port initialization to avoid requesting the > information again when changing the port configuration. > > The additional Rx queue will be assigned a PMD core like any other Rx > queue. Polling that extra queue may introduce increased latency and > a slight performance penalty at the benefit of preventing link flapping. > > This feature must be enabled per port on specific protocols via the > cp-protection option. This option takes a coma-separated list of > protocol names. It is only supported on ethernet ports. This feature is > experimental. > > If the user has already configured multiple Rx queues on the port, an > additional one will be allocated for control plane packets. If the > hardware cannot satisfy the requested number of requested Rx queues, the > last Rx queue will be assigned for control plane. If only one Rx queue > is available, the cp-protection feature will be disabled. If the > hardware does not support the RTE flow matchers/actions, the feature > will be disabled. > > It cannot be enabled when other_config:hw-offload=true as it may > conflict with the offloaded RTE flows. Similarly, if hw-offload is > enabled, cp-protection will be forcibly disabled on all ports. > > Example use: > > ovs-vsctl add-bond br-phy bond0 phy0 phy1 -- \ >set interface phy0 type=dpdk options:dpdk-devargs=:ca:00.0 -- \ >set interface phy0 options:cp-protection=lacp -- \ >set interface phy1 type=dpdk options:dpdk-devargs=:ca:00.1 -- \ >set interface phy1 options:cp-protection=lacp > > As a starting point, only one protocol is supported: LACP. Other > protocols can be added in the future. NIC compatibility should be > checked. > > To validate that this works as intended, I used a traffic generator to > generate random traffic slightly above the machine capacity at line rate > on a two ports bond interface. OVS is configured to receive traffic on > two VLANs and pop/push them in a br-int bridge based on tags set on > patch ports. > >+--+ >| DUT | >|++| >|| br-int || > in_port=patch10,actions=mod_dl_src:$patch11,mod_dl_dst:$tgen1,output:patch11 >|||| > in_port=patch11,actions=mod_dl_src:$patch10,mod_dl_dst:$tgen0,output:patch10 >|| patch10patch11 || >|+---|---|+| >|| | | >|+---|---|+| >|| patch00patch01 || >|| tag:10tag:20 || >|||| >|| br-phy || default flow, action=NORMAL >|||| >|| bond0|| balance-slb, lacp=passive, lacp-time=fast >||phy0 phy1 || >|+--|-|---+| >+---|-|+ >| | >+---|-|+ >| port0 port1 | balance L3/L4, lacp=active, lacp-time=fast >| lag | mode trunk VLANs 10, 20 >| | >|switch| >| | >| vlan 10vlan 20 | mode access >| port2 port3 | >+-|--|-+ > | | >+-|--|-+ >| tgen0 tgen1 | Random traffic that is properly balanced >| | across the bond ports in both directions. >| traffic generator | >+--+ > > Without cp-protection, the bond0 links are randomly switching to > "defaulted" when one of the LACP packets sent by the switch is dropped > because the RX queues are full and the PMD threads did not process them > fast enough. When that happens, all traffic must go through a single > link which causes above line rate traffic to be dropped. > > ~# ovs-appctl lacp/show-stats bond0 > -
Re: [ovs-dev] [PATCH v10] netdev-dpdk: add control plane protection support
On 23/05/2023 14:32, Aaron Conole wrote: Robin Jarry writes: Some control protocols are used to maintain link status between forwarding engines (e.g. LACP). When the system is not sized properly, the PMD threads may not be able to process all incoming traffic from the configured Rx queues. When a signaling packet of such protocols is dropped, it can cause link flapping, worsening the situation. I think one issue I have with this is that the name is a bit misleading. Control plane, from OVS perspective, would be like OpenFlow communications. This is more like a traffic steering mechanism. Maybe it would help to call it something like "traffic-based-rps" or something like that (but not clear what would be best). It is really a way to steer specific traffic to a distinct rxq. WDYT? I didn't look much at the code this time - maybe Kevin has taken a look? I'm looking :-) Use the RTE flow API to redirect these protocols into a dedicated Rx queue. The assumption is made that the ratio between control protocol traffic and user data traffic is very low and thus this dedicated Rx queue will never get full. The RSS redirection table is re-programmed to only use the other Rx queues. The RSS table size is stored in the netdev_dpdk structure at port initialization to avoid requesting the information again when changing the port configuration. The additional Rx queue will be assigned a PMD core like any other Rx queue. Polling that extra queue may introduce increased latency and a slight performance penalty at the benefit of preventing link flapping. This feature must be enabled per port on specific protocols via the cp-protection option. This option takes a coma-separated list of protocol names. It is only supported on ethernet ports. This feature is experimental. If the user has already configured multiple Rx queues on the port, an additional one will be allocated for control plane packets. If the hardware cannot satisfy the requested number of requested Rx queues, the last Rx queue will be assigned for control plane. If only one Rx queue is available, the cp-protection feature will be disabled. If the hardware does not support the RTE flow matchers/actions, the feature will be disabled. It cannot be enabled when other_config:hw-offload=true as it may conflict with the offloaded RTE flows. Similarly, if hw-offload is enabled, cp-protection will be forcibly disabled on all ports. Example use: ovs-vsctl add-bond br-phy bond0 phy0 phy1 -- \ set interface phy0 type=dpdk options:dpdk-devargs=:ca:00.0 -- \ set interface phy0 options:cp-protection=lacp -- \ set interface phy1 type=dpdk options:dpdk-devargs=:ca:00.1 -- \ set interface phy1 options:cp-protection=lacp As a starting point, only one protocol is supported: LACP. Other protocols can be added in the future. NIC compatibility should be checked. To validate that this works as intended, I used a traffic generator to generate random traffic slightly above the machine capacity at line rate on a two ports bond interface. OVS is configured to receive traffic on two VLANs and pop/push them in a br-int bridge based on tags set on patch ports. +--+ | DUT | |++| || br-int || in_port=patch10,actions=mod_dl_src:$patch11,mod_dl_dst:$tgen1,output:patch11 |||| in_port=patch11,actions=mod_dl_src:$patch10,mod_dl_dst:$tgen0,output:patch10 || patch10patch11 || |+---|---|+| || | | |+---|---|+| || patch00patch01 || || tag:10tag:20 || |||| || br-phy || default flow, action=NORMAL |||| || bond0|| balance-slb, lacp=passive, lacp-time=fast ||phy0 phy1 || |+--|-|---+| +---|-|+ | | +---|-|+ | port0 port1 | balance L3/L4, lacp=active, lacp-time=fast | lag | mode trunk VLANs 10, 20 | | |switch| | | | vlan 10vlan 20 | mode access | port2 port3 | +-|--|-+ | | +-|--|-+ | tgen0 tgen1 | Random traffic that is properly balanced | | across the bond ports in both directions. | traffic generator | +--+ Without cp-protection, the bond0 links are randomly switching to "defaulted" when one of the LACP packets sent by the switch is dropped because the RX queues are full and the PMD threads did not process them fast enough. When that happens, all traffic must go through a single link which causes above line rate traffic to be dropped. ~# ovs-appctl lacp/show-stats bond0 bond0 statistics member: phy0: TX PDUs: 347246 RX PDUs: 14
Re: [ovs-dev] [PATCH v10] netdev-dpdk: add control plane protection support
Aaron Conole, May 23, 2023 at 15:32: > I think one issue I have with this is that the name is a bit > misleading. Control plane, from OVS perspective, would be like OpenFlow > communications. This is more like a traffic steering mechanism. > > Maybe it would help to call it something like "traffic-based-rps" or > something like that (but not clear what would be best). It is really a > way to steer specific traffic to a distinct rxq. > > WDYT? Actually, "packet-steering" was one of the first ideas I had for this feature, but I thought it may be confusing. I am weary of reusing the "rps" acronym as it may be confused with a linux specific feature: https://docs.kernel.org/networking/scaling.html#rps-receive-packet-steering The feature introduced in this patch is relying on RTE flow which has nothing to do with Linux. However, I agree that the name should reflect that we are steering traffic into a dedicated rxq. How about these ideas? options:isolated-rxq=lacp,... options:rxq-isolate=lacp,... options:rxq-steering=lacp,... I personally prefer "rxq-isolate". ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v10] netdev-dpdk: add control plane protection support
On 5/23/23 16:16, Robin Jarry wrote: > Aaron Conole, May 23, 2023 at 15:32: >> I think one issue I have with this is that the name is a bit >> misleading. Control plane, from OVS perspective, would be like OpenFlow >> communications. This is more like a traffic steering mechanism. >> >> Maybe it would help to call it something like "traffic-based-rps" or >> something like that (but not clear what would be best). It is really a >> way to steer specific traffic to a distinct rxq. >> >> WDYT? > > Actually, "packet-steering" was one of the first ideas I had for this > feature, but I thought it may be confusing. I am weary of reusing the > "rps" acronym as it may be confused with a linux specific feature: > > https://docs.kernel.org/networking/scaling.html#rps-receive-packet-steering > > The feature introduced in this patch is relying on RTE flow which has > nothing to do with Linux. However, I agree that the name should reflect > that we are steering traffic into a dedicated rxq. How about these > ideas? > > options:isolated-rxq=lacp,... > options:rxq-isolate=lacp,... > options:rxq-steering=lacp,... > > I personally prefer "rxq-isolate". 'rxq-isolate' will be confused with 'other_config:pmd-rxq-isolate'. Same likely goes for the 'isolated-rxq'. 'rxq-steernig' may be confused with 'other_config:tx-steering'. But this can be argued that it's essentially similar functionality, so should be named similarly. Maybe we can double down on that and use options:rx-steernig=rss|rss+lacp|... with 'rss' being a default configuration? Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v10] netdev-dpdk: add control plane protection support
Ilya Maximets, May 23, 2023 at 22:04:
> 'rxq-isolate' will be confused with 'other_config:pmd-rxq-isolate'.
> Same likely goes for the 'isolated-rxq'.
>
> 'rxq-steernig' may be confused with 'other_config:tx-steering'.
> But this can be argued that it's essentially similar functionality,
> so should be named similarly. Maybe we can double down on that and
> use options:rx-steernig=rss|rss+lacp|... with 'rss' being a default
> configuration?
I like that idea. Instead of "rss", how about "hash" to match what
tx-steering is using?
So the value of options:rx-steering would be "hash" followed by an
arbitrary list of preset protocols (for now, only "lacp") all separated
by "+". It may also open the door for other base steering methods than
"hash" ("rr" for round-robin some day?).
If that is OK, I can prepare a v11 with everything renamed.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v10] netdev-dpdk: add control plane protection support
On 5/24/23 09:18, Robin Jarry wrote:
> Ilya Maximets, May 23, 2023 at 22:04:
>> 'rxq-isolate' will be confused with 'other_config:pmd-rxq-isolate'.
>> Same likely goes for the 'isolated-rxq'.
>>
>> 'rxq-steernig' may be confused with 'other_config:tx-steering'.
>> But this can be argued that it's essentially similar functionality,
>> so should be named similarly. Maybe we can double down on that and
>> use options:rx-steernig=rss|rss+lacp|... with 'rss' being a default
>> configuration?
>
> I like that idea. Instead of "rss", how about "hash" to match what
> tx-steering is using?
I'm not sure if it's better to use 'rss' or 'hash' in this context.
Aaron, Kevin, what do you think?
>
> So the value of options:rx-steering would be "hash" followed by an
> arbitrary list of preset protocols (for now, only "lacp") all separated
> by "+". It may also open the door for other base steering methods than
> "hash" ("rr" for round-robin some day?).
>
> If that is OK, I can prepare a v11 with everything renamed.
Might make sense for the code review. Kevin was looking at the
patch, IIUC.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v10] netdev-dpdk: add control plane protection support
On 24/05/2023 11:41, Ilya Maximets wrote:
On 5/24/23 09:18, Robin Jarry wrote:
Ilya Maximets, May 23, 2023 at 22:04:
'rxq-isolate' will be confused with 'other_config:pmd-rxq-isolate'.
Same likely goes for the 'isolated-rxq'.
'rxq-steernig' may be confused with 'other_config:tx-steering'.
But this can be argued that it's essentially similar functionality,
so should be named similarly. Maybe we can double down on that and
use options:rx-steernig=rss|rss+lacp|... with 'rss' being a default
configuration?
I like that idea. Instead of "rss", how about "hash" to match what
tx-steering is using?
I'm not sure if it's better to use 'rss' or 'hash' in this context.
Aaron, Kevin, what do you think?
Hmm, not sure on this one. I have a feeling that having a 'hash' mode
for tx-steering that only applies to vhost devices, and 'hash' mode for
rx-steering that only applies to NICs means people will miss the
subtlety and try to enable the wrong hash mode on the wrong device :-)
So 'rss' might make it more distinct.
So the value of options:rx-steering would be "hash" followed by an
arbitrary list of preset protocols (for now, only "lacp") all separated
by "+". It may also open the door for other base steering methods than
"hash" ("rr" for round-robin some day?).
If that is OK, I can prepare a v11 with everything renamed.
Any reason for '+' ? I think commas are used more frequently. If it
needed to be extended in future for some extra config, then adding
'key:value' pairs would be seamlessly. e.g.
=rss:, lacp:
Might make sense for the code review. Kevin was looking at the
patch, IIUC.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v10] netdev-dpdk: add control plane protection support
On 17/04/2023 13:37, Robin Jarry wrote: Some control protocols are used to maintain link status between forwarding engines (e.g. LACP). When the system is not sized properly, the PMD threads may not be able to process all incoming traffic from the configured Rx queues. When a signaling packet of such protocols is dropped, it can cause link flapping, worsening the situation. Use the RTE flow API to redirect these protocols into a dedicated Rx queue. The assumption is made that the ratio between control protocol traffic and user data traffic is very low and thus this dedicated Rx queue will never get full. The RSS redirection table is re-programmed to only use the other Rx queues. The RSS table size is stored in the netdev_dpdk structure at port initialization to avoid requesting the information again when changing the port configuration. The additional Rx queue will be assigned a PMD core like any other Rx queue. Polling that extra queue may introduce increased latency and a slight performance penalty at the benefit of preventing link flapping. This feature must be enabled per port on specific protocols via the cp-protection option. This option takes a coma-separated list of protocol names. It is only supported on ethernet ports. This feature is experimental. If the user has already configured multiple Rx queues on the port, an additional one will be allocated for control plane packets. If the hardware cannot satisfy the requested number of requested Rx queues, the last Rx queue will be assigned for control plane. If only one Rx queue is available, the cp-protection feature will be disabled. If the hardware does not support the RTE flow matchers/actions, the feature will be disabled. It cannot be enabled when other_config:hw-offload=true as it may conflict with the offloaded RTE flows. Similarly, if hw-offload is enabled, cp-protection will be forcibly disabled on all ports. Example use: ovs-vsctl add-bond br-phy bond0 phy0 phy1 -- \ set interface phy0 type=dpdk options:dpdk-devargs=:ca:00.0 -- \ set interface phy0 options:cp-protection=lacp -- \ set interface phy1 type=dpdk options:dpdk-devargs=:ca:00.1 -- \ set interface phy1 options:cp-protection=lacp As a starting point, only one protocol is supported: LACP. Other protocols can be added in the future. NIC compatibility should be checked. To validate that this works as intended, I used a traffic generator to generate random traffic slightly above the machine capacity at line rate on a two ports bond interface. OVS is configured to receive traffic on two VLANs and pop/push them in a br-int bridge based on tags set on patch ports. +--+ | DUT | |++| || br-int || in_port=patch10,actions=mod_dl_src:$patch11,mod_dl_dst:$tgen1,output:patch11 |||| in_port=patch11,actions=mod_dl_src:$patch10,mod_dl_dst:$tgen0,output:patch10 || patch10patch11 || |+---|---|+| || | | |+---|---|+| || patch00patch01 || || tag:10tag:20 || |||| || br-phy || default flow, action=NORMAL |||| || bond0|| balance-slb, lacp=passive, lacp-time=fast ||phy0 phy1 || |+--|-|---+| +---|-|+ | | +---|-|+ | port0 port1 | balance L3/L4, lacp=active, lacp-time=fast | lag | mode trunk VLANs 10, 20 | | |switch| | | | vlan 10vlan 20 | mode access | port2 port3 | +-|--|-+ | | +-|--|-+ | tgen0 tgen1 | Random traffic that is properly balanced | | across the bond ports in both directions. | traffic generator | +--+ Without cp-protection, the bond0 links are randomly switching to "defaulted" when one of the LACP packets sent by the switch is dropped because the RX queues are full and the PMD threads did not process them fast enough. When that happens, all traffic must go through a single link which causes above line rate traffic to be dropped. ~# ovs-appctl lacp/show-stats bond0 bond0 statistics member: phy0: TX PDUs: 347246 RX PDUs: 14865 RX Bad PDUs: 0 RX Marker Request PDUs: 0 Link Expired: 168 Link Defaulted: 0 Carrier Status Changed: 0 member: phy1: TX PDUs: 347245 RX PDUs: 14919 RX Bad PDUs: 0 RX Marker Request PDUs: 0 Link Expired: 147 Link Defaulted: 1 Carrier Status Changed: 0 When cp-protection is enabled, no LACP packet is dropped and the bond links remain enabled at all times, maximizing the throughput. Neither the "Link Expired" nor the "Link Defaulted" counters are incremented an
Re: [ovs-dev] [PATCH v10] netdev-dpdk: add control plane protection support
Kevin Traynor, May 24, 2023 at 16:06: > Hmm, not sure on this one. I have a feeling that having a 'hash' mode > for tx-steering that only applies to vhost devices, and 'hash' mode > for rx-steering that only applies to NICs means people will miss the > subtlety and try to enable the wrong hash mode on the wrong device :-) > So 'rss' might make it more distinct. "rss" is fine then. > Any reason for '+' ? I think commas are used more frequently. If it > needed to be extended in future for some extra config, then adding > 'key:value' pairs would be seamlessly. e.g. =rss:, > lacp: The "rss" item should be mandatory anyway. There should be no way to disable it. I assume that it is what Ilya meant with these "+" symbols. That would leave us with these examples: # lacp+bfd on separate queue, rss on other queues options:rx-steering=rss,lacp,bfd # same options:rx-steering=lacp,bfd,rss # only regular rss, same as no config at all options:rx-steering=rss # invalid configurations options:rx-steering=lacp options:rx-steering= options:rx-steering=bfd,lacp What do you think? ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v10] netdev-dpdk: add control plane protection support
On 24/05/2023 15:32, Robin Jarry wrote: Kevin Traynor, May 24, 2023 at 16:06: Hmm, not sure on this one. I have a feeling that having a 'hash' mode for tx-steering that only applies to vhost devices, and 'hash' mode for rx-steering that only applies to NICs means people will miss the subtlety and try to enable the wrong hash mode on the wrong device :-) So 'rss' might make it more distinct. "rss" is fine then. Any reason for '+' ? I think commas are used more frequently. If it needed to be extended in future for some extra config, then adding 'key:value' pairs would be seamlessly. e.g. =rss:, lacp: The "rss" item should be mandatory anyway. There should be no way to disable it. I assume that it is what Ilya meant with these "+" symbols. That would leave us with these examples: # lacp+bfd on separate queue, rss on other queues options:rx-steering=rss,lacp,bfd # same options:rx-steering=lacp,bfd,rss ^ It looks a little odd to me that some values are for single queues and some are for the rest of the queues, with no way to distinguish. So maybe Ilya had placed significance in the order with his suggestion ? i.e. [default]+[single queue proto]+[other single queue proto]+... I don't want to bike shed too much on it, i guess with good docs, it can be clear anyway. # only regular rss, same as no config at all options:rx-steering=rss # invalid configurations options:rx-steering=lacp options:rx-steering= (^ This will be ok and use the default rss) options:rx-steering=bfd,lacp What do you think? ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v10] netdev-dpdk: add control plane protection support
Hey Kevin,
Kevin Traynor, May 24, 2023 at 16:13:
> Hi Robin,
>
> I tested combinations of enabling/disabling cp-proto and enabling
> hwol. It is working as expected and hwol feature always has a clear
> priority, regardless of the order they are enabled in.
>
> I didn't test lacp traffic, but I was able to see that the rxq was
> added/removed for it and that rss was working on the other rxqs.
>
> I also tested combinations of different numbers of rxqs,
> adding/deleting rxqs, enabling/disabling cp-proto etc and working
> fine. I also tested enabling cp-proto and changing rxqs in the same
> command, working fine.
Thanks for testing!
> > +/* User input for n_rxq + an optional control plane protection
> > queue
> > + * (see netdev_dpdk_reconfigure). This field is different from the
> > + * other requested_* fields as it may contain a different value
> > than
> > + * the user input. */
>
> ^ I don't think this comment is really needed. requested_rxq_size can
> also be adjusted and they don't always come from user input, sometimes
> just the OVS default.
...
> > +/* User input for n_rxq (see dpdk_set_rxq_config). */
> > +int user_n_rxq;
>
> I also put in a similar 3rd stage status in recent patches for
> rx/tx-descriptors but managed to get rid of it. You might be able to
> remove this and just check check cp-proto flags do a +1/-1 where
> needed?
I had already tried this and it was not possible. I will have another
fresh look. Maybe I was thinking about this the wrong way.
> > +/*
> > + * Some drivers set reta_size equal to the total number of rxqs
> > that
> > + * are configured when it is a power of two. Since we are actually
> > + * reconfiguring the redirection table to exclude the last rxq, we
> > may
> > + * end up with an imbalanced redirection table. For example, such
> > + * configuration:
> > + *
> > + * options:n_rxq=3 options:cp-protection=lacp
> > + *
> > + * Will actually configure 4 rxqs on the NIC, and the default reta
> > to:
> > + *
> > + * [0, 1, 2, 3]
> > + *
> > + * And dpdk_cp_prot_rss_configure() will reconfigure reta to:
> > + *
> > + * [0, 1, 2, 0]
> > + *
> > + * Causing queue 0 to receive twice as much traffic as queues 1
> > and 2.
> > + *
> > + * Work around that corner case by forcing a bigger redirection
> > table
> > + * size to 128 entries when reta_size is not a multiple of
> > rss_n_rxq
> > + * and when reta_size is less than 128. This value seems to be
> > + * supported by most of the drivers that also support rte flow.
> > + */
> > +info.reta_size = RTE_ETH_RSS_RETA_SIZE_128;
>
> Thanks for following up on previous comments and adding this
> workaround. I think you didn't get much response about it, so might be
> worth adding a DPDK bugzilla to formally report/track it.
I don't think it is a bug. If the default redirection table is not
modified, there will be proper balancing on all Rx queues. The issue we
have here is that we are reconfiguring the table and removing the last
queue from it which can lead to imbalances in some cases. Do you think
this is worth reporting anyway?
> > +try_cp_prot = dev->requested_cp_prot_flags != 0;
> > +dev->requested_n_rxq = dev->user_n_rxq;
> > +if (try_cp_prot) {
> > +dev->requested_n_rxq += 1;
>
> Minor, but adjusting the requested_n_rxq's seems more suited to
> dpdk_cp_prot_set_config() (and move it after dpdk_set_rxq_config() in
> netdev_dpdk_set_config()) as this is the function that applies the
> configuration. I admit, it's partly aesthetic, so that this function
> still just checks for changes that require a reconfig and returns if
> there are none :-)
This is related to the introduction of user_n_rxq. I had tried to make
it work and this was the only way. I'll have another fresh look.
Cheers,
Robin
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v10] netdev-dpdk: add control plane protection support
On 5/24/23 16:54, Kevin Traynor wrote: > On 24/05/2023 15:32, Robin Jarry wrote: >> Kevin Traynor, May 24, 2023 at 16:06: >>> Hmm, not sure on this one. I have a feeling that having a 'hash' mode >>> for tx-steering that only applies to vhost devices, and 'hash' mode >>> for rx-steering that only applies to NICs means people will miss the >>> subtlety and try to enable the wrong hash mode on the wrong device :-) >>> So 'rss' might make it more distinct. >> >> "rss" is fine then. >> >>> Any reason for '+' ? I think commas are used more frequently. If it >>> needed to be extended in future for some extra config, then adding >>> 'key:value' pairs would be seamlessly. e.g. =rss:, >>> lacp: >> >> The "rss" item should be mandatory anyway. There should be no way to >> disable it. I assume that it is what Ilya meant with these "+" symbols. >> That would leave us with these examples: >> >> # lacp+bfd on separate queue, rss on other queues >> >> options:rx-steering=rss,lacp,bfd >> >> # same >> >> options:rx-steering=lacp,bfd,rss >> > > ^ It looks a little odd to me that some values are for single queues and > some are for the rest of the queues, with no way to distinguish. > > So maybe Ilya had placed significance in the order with his suggestion ? > i.e. [default]+[single queue proto]+[other single queue proto]+... I had a '+' because rss and lacp are two different entities and I looked at it as a mode of operation. i.e. RSS plus special handling for LACP. RSS looks strange in a comma-separated list, IMO. > > I don't want to bike shed too much on it, i guess with good docs, it can > be clear anyway. > >> # only regular rss, same as no config at all >> >> options:rx-steering=rss >> >> # invalid configurations >> >> options:rx-steering=lacp > >> options:rx-steering= > (^ This will be ok and use the default rss) > >> options:rx-steering=bfd,lacp >> >> What do you think? >> > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v10] netdev-dpdk: add control plane protection support
Kevin Traynor, May 24, 2023 at 16:54: > > The "rss" item should be mandatory anyway. There should be no way to > > disable it. I assume that it is what Ilya meant with these "+" symbols. > > That would leave us with these examples: > > > > # lacp+bfd on separate queue, rss on other queues > > > > options:rx-steering=rss,lacp,bfd > > > > # same > > > > options:rx-steering=lacp,bfd,rss > > ^ It looks a little odd to me that some values are for single queues and > some are for the rest of the queues, with no way to distinguish. > > So maybe Ilya had placed significance in the order with his suggestion ? > i.e. [default]+[single queue proto]+[other single queue proto]+... > > I don't want to bike shed too much on it, i guess with good docs, it can > be clear anyway. > > > # only regular rss, same as no config at all > > > > options:rx-steering=rss > > > > # invalid configurations > > > > options:rx-steering=lacp > > > options:rx-steering= > (^ This will be ok and use the default rss) In that case, maybe we should exclude "rss" from the available items to avoid confusion and only require users to specify the protocols that need to be put in a separate queue. The remaining queues being rss by default. Would that be ok? ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v10] netdev-dpdk: add control plane protection support
On 24/05/2023 16:05, Robin Jarry wrote: Kevin Traynor, May 24, 2023 at 16:54: The "rss" item should be mandatory anyway. There should be no way to disable it. I assume that it is what Ilya meant with these "+" symbols. That would leave us with these examples: # lacp+bfd on separate queue, rss on other queues options:rx-steering=rss,lacp,bfd # same options:rx-steering=lacp,bfd,rss ^ It looks a little odd to me that some values are for single queues and some are for the rest of the queues, with no way to distinguish. So maybe Ilya had placed significance in the order with his suggestion ? i.e. [default]+[single queue proto]+[other single queue proto]+... I don't want to bike shed too much on it, i guess with good docs, it can be clear anyway. # only regular rss, same as no config at all options:rx-steering=rss # invalid configurations options:rx-steering=lacp options:rx-steering= (^ This will be ok and use the default rss) In that case, maybe we should exclude "rss" from the available items to avoid confusion and only require users to specify the protocols that need to be put in a separate queue. The remaining queues being rss by default. Would that be ok? Ilya explained the reasoning behind his idea in other mail and it seems fine, so for the sake of progress, I'd say to go with with his idea if you don't object to it. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v10] netdev-dpdk: add control plane protection support
Hey Ilya, Ilya Maximets, May 24, 2023 at 17:05: > I had a '+' because rss and lacp are two different entities and I looked > at it as a mode of operation. i.e. RSS plus special handling for LACP. > RSS looks strange in a comma-separated list, IMO. For now, there is only LACP but if other protocols are added (e.g. BFD), wouldn't it be weird to have them separated as well? options:rx-steering=rss+lacp+bfd Since lacp and bfd will most likely be put in the additional rxq, it would make sense to identify them as a group. I also have other reserves about specifying rss here after thinking some more about it: - rss shouldn't be disabled anyway, this forces users to always specify it. This is not great from a usability point of view. - When there is a single rxq configured by the user, there is no RSS happening per-se since all other traffic will be put in a single queue. The additional rxq being reserved for lacp and/or other special traffic. What do you think about removing "rss" altogether from the items? options:rx-steering=lacp,bfd,... Cheers. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v10] netdev-dpdk: add control plane protection support
On 5/25/23 11:25, Robin Jarry wrote:
> Hey Ilya,
>
> Ilya Maximets, May 24, 2023 at 17:05:
>> I had a '+' because rss and lacp are two different entities and I looked
>> at it as a mode of operation. i.e. RSS plus special handling for LACP.
>> RSS looks strange in a comma-separated list, IMO.
>
> For now, there is only LACP but if other protocols are added (e.g. BFD),
> wouldn't it be weird to have them separated as well?
>
> options:rx-steering=rss+lacp+bfd
>
> Since lacp and bfd will most likely be put in the additional rxq, it
> would make sense to identify them as a group.
>
> I also have other reserves about specifying rss here after thinking some
> more about it:
>
> - rss shouldn't be disabled anyway, this forces users to always specify
> it. This is not great from a usability point of view.
>
> - When there is a single rxq configured by the user, there is no RSS
> happening per-se since all other traffic will be put in a single
> queue. The additional rxq being reserved for lacp and/or other special
> traffic.
>
> What do you think about removing "rss" altogether from the items?
>
> options:rx-steering=lacp,bfd,...
IMO, 'rx-steering=lacp' is not descriptive. By looking at it users can't
tell what is going on without reading the documentation. So, I think,
the 'rss' part is necessary.
Do you anticipate protocols other than lacp and bfd? I'd actually just
treat them as enum ('rss', 'rss+lacp', 'rss+bfd' and 'rss+lacp+bfd') and
not a list. And document them as a enum. We can reconsider in the future
if we'll need more than 2 protocols. It's experimental for now anyway.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
