| Message ID | 20220117222521.771593-1-maxime.coquelin@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] vswitchd.xml: Add missing tx-steering PMD option. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On 17/01/2022 22:25, Maxime Coquelin wrote: > This patch documents PMD's other_config:tx-steering option. > > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> > --- > vswitchd/vswitch.xml | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml > index 026b5e2ca..ef7f8f2c8 100644 > --- a/vswitchd/vswitch.xml > +++ b/vswitchd/vswitch.xml > @@ -3375,6 +3375,28 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \ > </ul> > <p>This option may only be used with dpdk VF representors.</p> > </column> > + > + <column name="other_config" key="tx-steering" > + type='{"type": "string", > + "enum": ["set", ["thread", "hash"]]}'> > + <p> > + Specifies the Tx steering mode for the interface. > + </p> > + <p> > + <code>thread</code> enables static txq mapping when the number of txq > + is greater or equal than the number of PMD threads, and XPS mode if > + lower than the number of PMD threads. I think it should be 'greater than PMD threads, and XPS mode if equal or lower than the number of PMD threads.', because one txq is also needed for the OVS main thread. I checked the code and this txq is accounted for in wanted_txqs. With this fix: Acked-by: Kevin Traynor <ktraynor@redhat.com> > + </p> > + <p> > + <code>hash</code> enables hash-based Tx steering, which distributes > + the packets on all the transmit queues based on their 5-tuples > + hashes. > + </p> > + <p> > + Defaults to <code>thread</code>. > + </p> > + </column> > + > </group> > > <group title="EMC (Exact Match Cache) Configuration"> >
On 1/18/22 11:51, Kevin Traynor wrote: > On 17/01/2022 22:25, Maxime Coquelin wrote: >> This patch documents PMD's other_config:tx-steering option. >> >> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> >> --- >> vswitchd/vswitch.xml | 22 ++++++++++++++++++++++ >> 1 file changed, 22 insertions(+) >> >> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml >> index 026b5e2ca..ef7f8f2c8 100644 >> --- a/vswitchd/vswitch.xml >> +++ b/vswitchd/vswitch.xml >> @@ -3375,6 +3375,28 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \ >> </ul> >> <p>This option may only be used with dpdk VF representors.</p> >> </column> >> + >> + <column name="other_config" key="tx-steering" >> + type='{"type": "string", >> + "enum": ["set", ["thread", "hash"]]}'> >> + <p> >> + Specifies the Tx steering mode for the interface. >> + </p> >> + <p> >> + <code>thread</code> enables static txq mapping when the number of txq >> + is greater or equal than the number of PMD threads, and XPS mode if >> + lower than the number of PMD threads. > > I think it should be 'greater than PMD threads, and XPS mode if equal or lower > than the number of PMD threads.', because one txq is also needed for the OVS > main thread. I checked the code and this txq is accounted for in wanted_txqs. I would also recommend to not use the 'XPS' as a name for the dynamic txq mapping in the 'thread' mode. Because all of this is a sort of XPS. The hash mode is a form of {trans,x}mit packet steering too. So, something like this maybe: <code>thread</code> enables static (1:1) thread-to-txq mapping when the number of Tx queues is greater than number of PMD threads, and dynamic (N:1) mapping if equal or lower. In this mode a single thread can not use more than 1 transmit queue of a given port. And, I think, we need to fix the userspace-tx-steering.rst too, e.g.: Depending on the port's number of Tx queues being greater or equal than the number of PMD threads, static (1:1) or dynamic (N:1) thread-to-txq mapping will be used. Maxime, Kevin, what do you think? > > With this fix: > Acked-by: Kevin Traynor <ktraynor@redhat.com> > >> + </p> >> + <p> >> + <code>hash</code> enables hash-based Tx steering, which distributes >> + the packets on all the transmit queues based on their 5-tuples >> + hashes. >> + </p> >> + <p> >> + Defaults to <code>thread</code>. >> + </p> >> + </column> >> + >> </group> >> <group title="EMC (Exact Match Cache) Configuration"> >> >
Hi, Ilya, Kevin, On 1/20/22 13:16, Ilya Maximets wrote: > On 1/18/22 11:51, Kevin Traynor wrote: >> On 17/01/2022 22:25, Maxime Coquelin wrote: >>> This patch documents PMD's other_config:tx-steering option. >>> >>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> >>> --- >>> vswitchd/vswitch.xml | 22 ++++++++++++++++++++++ >>> 1 file changed, 22 insertions(+) >>> >>> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml >>> index 026b5e2ca..ef7f8f2c8 100644 >>> --- a/vswitchd/vswitch.xml >>> +++ b/vswitchd/vswitch.xml >>> @@ -3375,6 +3375,28 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \ >>> </ul> >>> <p>This option may only be used with dpdk VF representors.</p> >>> </column> >>> + >>> + <column name="other_config" key="tx-steering" >>> + type='{"type": "string", >>> + "enum": ["set", ["thread", "hash"]]}'> >>> + <p> >>> + Specifies the Tx steering mode for the interface. >>> + </p> >>> + <p> >>> + <code>thread</code> enables static txq mapping when the number of txq >>> + is greater or equal than the number of PMD threads, and XPS mode if >>> + lower than the number of PMD threads. >> >> I think it should be 'greater than PMD threads, and XPS mode if equal or lower >> than the number of PMD threads.', because one txq is also needed for the OVS >> main thread. I checked the code and this txq is accounted for in wanted_txqs. > > I would also recommend to not use the 'XPS' as a name for the dynamic txq mapping > in the 'thread' mode. Because all of this is a sort of XPS. The hash mode is > a form of {trans,x}mit packet steering too. > So, something like this maybe: > > <code>thread</code> enables static (1:1) thread-to-txq mapping when the > number of Tx queues is greater than number of PMD threads, and dynamic (N:1) > mapping if equal or lower. In this mode a single thread can not use more > than 1 transmit queue of a given port. > > And, I think, we need to fix the userspace-tx-steering.rst too, e.g.: > > Depending on the port's number of Tx queues being greater or equal than the > number of PMD threads, static (1:1) or dynamic (N:1) thread-to-txq mapping > will be used. > > Maxime, Kevin, what do you think? I agree with your suggestions, I'll post a new revision tomorrow, that will include userspace-tx-steering.rst changes too. Thanks, Maxime >> >> With this fix: >> Acked-by: Kevin Traynor <ktraynor@redhat.com> >> >>> + </p> >>> + <p> >>> + <code>hash</code> enables hash-based Tx steering, which distributes >>> + the packets on all the transmit queues based on their 5-tuples >>> + hashes. >>> + </p> >>> + <p> >>> + Defaults to <code>thread</code>. >>> + </p> >>> + </column> >>> + >>> </group> >>> <group title="EMC (Exact Match Cache) Configuration"> >>> >> >
On 20/01/2022 12:16, Ilya Maximets wrote: > On 1/18/22 11:51, Kevin Traynor wrote: >> On 17/01/2022 22:25, Maxime Coquelin wrote: >>> This patch documents PMD's other_config:tx-steering option. >>> >>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> >>> --- >>> vswitchd/vswitch.xml | 22 ++++++++++++++++++++++ >>> 1 file changed, 22 insertions(+) >>> >>> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml >>> index 026b5e2ca..ef7f8f2c8 100644 >>> --- a/vswitchd/vswitch.xml >>> +++ b/vswitchd/vswitch.xml >>> @@ -3375,6 +3375,28 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \ >>> </ul> >>> <p>This option may only be used with dpdk VF representors.</p> >>> </column> >>> + >>> + <column name="other_config" key="tx-steering" >>> + type='{"type": "string", >>> + "enum": ["set", ["thread", "hash"]]}'> >>> + <p> >>> + Specifies the Tx steering mode for the interface. >>> + </p> >>> + <p> >>> + <code>thread</code> enables static txq mapping when the number of txq >>> + is greater or equal than the number of PMD threads, and XPS mode if >>> + lower than the number of PMD threads. >> >> I think it should be 'greater than PMD threads, and XPS mode if equal or lower >> than the number of PMD threads.', because one txq is also needed for the OVS >> main thread. I checked the code and this txq is accounted for in wanted_txqs. > > I would also recommend to not use the 'XPS' as a name for the dynamic txq mapping > in the 'thread' mode. Because all of this is a sort of XPS. The hash mode is > a form of {trans,x}mit packet steering too. > So, something like this maybe: > > <code>thread</code> enables static (1:1) thread-to-txq mapping when the > number of Tx queues is greater than number of PMD threads, and dynamic (N:1) > mapping if equal or lower. In this mode a single thread can not use more > than 1 transmit queue of a given port. > > And, I think, we need to fix the userspace-tx-steering.rst too, e.g.: > > Depending on the port's number of Tx queues being greater or equal than the > number of PMD threads, static (1:1) or dynamic (N:1) thread-to-txq mapping > will be used. > > Maxime, Kevin, what do you think? > +1. Only thing I'd add is to explicitly associate each mode with the txq/PMD values in the rst, like it is done in the xml. >> >> With this fix: >> Acked-by: Kevin Traynor <ktraynor@redhat.com> >> >>> + </p> >>> + <p> >>> + <code>hash</code> enables hash-based Tx steering, which distributes >>> + the packets on all the transmit queues based on their 5-tuples >>> + hashes. >>> + </p> >>> + <p> >>> + Defaults to <code>thread</code>. >>> + </p> >>> + </column> >>> + >>> </group> >>> <group title="EMC (Exact Match Cache) Configuration"> >>> >> >
On 1/20/22 14:20, Kevin Traynor wrote: > On 20/01/2022 12:16, Ilya Maximets wrote: >> On 1/18/22 11:51, Kevin Traynor wrote: >>> On 17/01/2022 22:25, Maxime Coquelin wrote: >>>> This patch documents PMD's other_config:tx-steering option. >>>> >>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> >>>> --- >>>> vswitchd/vswitch.xml | 22 ++++++++++++++++++++++ >>>> 1 file changed, 22 insertions(+) >>>> >>>> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml >>>> index 026b5e2ca..ef7f8f2c8 100644 >>>> --- a/vswitchd/vswitch.xml >>>> +++ b/vswitchd/vswitch.xml >>>> @@ -3375,6 +3375,28 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 >>>> type=patch options:peer=p1 \ >>>> </ul> >>>> <p>This option may only be used with dpdk VF >>>> representors.</p> >>>> </column> >>>> + >>>> + <column name="other_config" key="tx-steering" >>>> + type='{"type": "string", >>>> + "enum": ["set", ["thread", "hash"]]}'> >>>> + <p> >>>> + Specifies the Tx steering mode for the interface. >>>> + </p> >>>> + <p> >>>> + <code>thread</code> enables static txq mapping when the >>>> number of txq >>>> + is greater or equal than the number of PMD threads, and >>>> XPS mode if >>>> + lower than the number of PMD threads. >>> >>> I think it should be 'greater than PMD threads, and XPS mode if equal >>> or lower >>> than the number of PMD threads.', because one txq is also needed for >>> the OVS >>> main thread. I checked the code and this txq is accounted for in >>> wanted_txqs. >> >> I would also recommend to not use the 'XPS' as a name for the dynamic >> txq mapping >> in the 'thread' mode. Because all of this is a sort of XPS. The hash >> mode is >> a form of {trans,x}mit packet steering too. >> So, something like this maybe: >> >> <code>thread</code> enables static (1:1) thread-to-txq mapping when >> the >> number of Tx queues is greater than number of PMD threads, and >> dynamic (N:1) >> mapping if equal or lower. In this mode a single thread can not >> use more >> than 1 transmit queue of a given port. >> >> And, I think, we need to fix the userspace-tx-steering.rst too, e.g.: >> >> Depending on the port's number of Tx queues being greater or equal >> than the >> number of PMD threads, static (1:1) or dynamic (N:1) thread-to-txq >> mapping >> will be used. >> >> Maxime, Kevin, what do you think? >> > > +1. Only thing I'd add is to explicitly associate each mode with the > txq/PMD values in the rst, like it is done in the xml. Agree, I'm changing the paragraph to: " Thread mode enables static (1:1) thread-to-txq mapping when the number of Tx queues is greater than number of PMD threads, and dynamic (N:1) mapping if equal or lower. In this mode a single thread can not use more than 1 transmit queue of a given port. This is the recommended mode for performance reasons if the number of Tx queues is greater than the number of PMD threads, because the Tx lock is not acquired. If the number of Tx queues is greater than the number of threads (including the main thread), the remaining Tx queues will not be used. " Sounds good to you? Thanks, Maxime >>> >>> With this fix: >>> Acked-by: Kevin Traynor <ktraynor@redhat.com> >>> >>>> + </p> >>>> + <p> >>>> + <code>hash</code> enables hash-based Tx steering, which >>>> distributes >>>> + the packets on all the transmit queues based on their >>>> 5-tuples >>>> + hashes. >>>> + </p> >>>> + <p> >>>> + Defaults to <code>thread</code>. >>>> + </p> >>>> + </column> >>>> + >>>> </group> >>>> <group title="EMC (Exact Match Cache) Configuration"> >>>> >>> >> >
On 24/01/2022 16:00, Maxime Coquelin wrote: > > > On 1/20/22 14:20, Kevin Traynor wrote: >> On 20/01/2022 12:16, Ilya Maximets wrote: >>> On 1/18/22 11:51, Kevin Traynor wrote: >>>> On 17/01/2022 22:25, Maxime Coquelin wrote: >>>>> This patch documents PMD's other_config:tx-steering option. >>>>> >>>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> >>>>> --- >>>>> vswitchd/vswitch.xml | 22 ++++++++++++++++++++++ >>>>> 1 file changed, 22 insertions(+) >>>>> >>>>> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml >>>>> index 026b5e2ca..ef7f8f2c8 100644 >>>>> --- a/vswitchd/vswitch.xml >>>>> +++ b/vswitchd/vswitch.xml >>>>> @@ -3375,6 +3375,28 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 >>>>> type=patch options:peer=p1 \ >>>>> </ul> >>>>> <p>This option may only be used with dpdk VF >>>>> representors.</p> >>>>> </column> >>>>> + >>>>> + <column name="other_config" key="tx-steering" >>>>> + type='{"type": "string", >>>>> + "enum": ["set", ["thread", "hash"]]}'> >>>>> + <p> >>>>> + Specifies the Tx steering mode for the interface. >>>>> + </p> >>>>> + <p> >>>>> + <code>thread</code> enables static txq mapping when the >>>>> number of txq >>>>> + is greater or equal than the number of PMD threads, and >>>>> XPS mode if >>>>> + lower than the number of PMD threads. >>>> >>>> I think it should be 'greater than PMD threads, and XPS mode if equal >>>> or lower >>>> than the number of PMD threads.', because one txq is also needed for >>>> the OVS >>>> main thread. I checked the code and this txq is accounted for in >>>> wanted_txqs. >>> >>> I would also recommend to not use the 'XPS' as a name for the dynamic >>> txq mapping >>> in the 'thread' mode. Because all of this is a sort of XPS. The hash >>> mode is >>> a form of {trans,x}mit packet steering too. >>> So, something like this maybe: >>> >>> <code>thread</code> enables static (1:1) thread-to-txq mapping when >>> the >>> number of Tx queues is greater than number of PMD threads, and >>> dynamic (N:1) >>> mapping if equal or lower. In this mode a single thread can not >>> use more >>> than 1 transmit queue of a given port. >>> >>> And, I think, we need to fix the userspace-tx-steering.rst too, e.g.: >>> >>> Depending on the port's number of Tx queues being greater or equal >>> than the >>> number of PMD threads, static (1:1) or dynamic (N:1) thread-to-txq >>> mapping >>> will be used. >>> >>> Maxime, Kevin, what do you think? >>> >> >> +1. Only thing I'd add is to explicitly associate each mode with the >> txq/PMD values in the rst, like it is done in the xml. > > Agree, I'm changing the paragraph to: > " > Thread mode enables static (1:1) thread-to-txq mapping when the number of Tx > queues is greater than number of PMD threads, and dynamic (N:1) mapping if > equal or lower. In this mode a single thread can not use more than 1 > transmit > queue of a given port. > > This is the recommended mode for performance reasons if the number of Tx > queues > is greater than the number of PMD threads, because the Tx lock is not > acquired. > > If the number of Tx queues is greater than the number of threads > (including the > main thread), the remaining Tx queues will not be used. > " > > Sounds good to you? > LGTM. You can add my Ack on the updated patch. Thanks Maxime. Acked-by: Kevin Traynor <ktraynor@redhat.com> > Thanks, > Maxime > >>>> >>>> With this fix: >>>> Acked-by: Kevin Traynor <ktraynor@redhat.com> >>>> >>>>> + </p> >>>>> + <p> >>>>> + <code>hash</code> enables hash-based Tx steering, which >>>>> distributes >>>>> + the packets on all the transmit queues based on their >>>>> 5-tuples >>>>> + hashes. >>>>> + </p> >>>>> + <p> >>>>> + Defaults to <code>thread</code>. >>>>> + </p> >>>>> + </column> >>>>> + >>>>> </group> >>>>> <group title="EMC (Exact Match Cache) Configuration"> >>>>> >>>> >>> >> >
diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml index 026b5e2ca..ef7f8f2c8 100644 --- a/vswitchd/vswitch.xml +++ b/vswitchd/vswitch.xml @@ -3375,6 +3375,28 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \ </ul> <p>This option may only be used with dpdk VF representors.</p> </column> + + <column name="other_config" key="tx-steering" + type='{"type": "string", + "enum": ["set", ["thread", "hash"]]}'> + <p> + Specifies the Tx steering mode for the interface. + </p> + <p> + <code>thread</code> enables static txq mapping when the number of txq + is greater or equal than the number of PMD threads, and XPS mode if + lower than the number of PMD threads. + </p> + <p> + <code>hash</code> enables hash-based Tx steering, which distributes + the packets on all the transmit queues based on their 5-tuples + hashes. + </p> + <p> + Defaults to <code>thread</code>. + </p> + </column> + </group> <group title="EMC (Exact Match Cache) Configuration">
This patch documents PMD's other_config:tx-steering option. Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> --- vswitchd/vswitch.xml | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)