[ovs-dev] vswitch.xml: Fix column for xdpmode.
diff mbox series

Message ID 20191105205408.22071-1-i.maximets@ovn.org
State Accepted
Headers show
Series
  • [ovs-dev] vswitch.xml: Fix column for xdpmode.
Related show

Commit Message

Ilya Maximets Nov. 5, 2019, 8:54 p.m. UTC
'xdpmode' is part of 'options', not the 'other_config'.

CC: William Tu <u9012063@gmail.com>
Fixes: 0de1b425962d ("netdev-afxdp: add new netdev type for AF_XDP.")
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
---
 vswitchd/vswitch.xml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Ben Pfaff Nov. 5, 2019, 9:20 p.m. UTC | #1
On Tue, Nov 05, 2019 at 09:54:08PM +0100, Ilya Maximets wrote:
> 'xdpmode' is part of 'options', not the 'other_config'.
> 
> CC: William Tu <u9012063@gmail.com>
> Fixes: 0de1b425962d ("netdev-afxdp: add new netdev type for AF_XDP.")
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>

It's not clear to me whether the options related to XDP and DPDK and PMD
consistently follow the intended convention.  The intention is that
options is for settings that are specific to a particular configured
interface type, and other-config is for options independent of the
interface type.

Acked-by: Ben Pfaff <blp@ovn.org>
William Tu Nov. 5, 2019, 11:07 p.m. UTC | #2
On Tue, Nov 05, 2019 at 01:20:35PM -0800, Ben Pfaff wrote:
> On Tue, Nov 05, 2019 at 09:54:08PM +0100, Ilya Maximets wrote:
> > 'xdpmode' is part of 'options', not the 'other_config'.
> > 
> > CC: William Tu <u9012063@gmail.com>
> > Fixes: 0de1b425962d ("netdev-afxdp: add new netdev type for AF_XDP.")
> > Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> 
> It's not clear to me whether the options related to XDP and DPDK and PMD
> consistently follow the intended convention.  The intention is that
> options is for settings that are specific to a particular configured
> interface type, and other-config is for options independent of the
> interface type.
> 
> Acked-by: Ben Pfaff <blp@ovn.org>

Thank you, I applied to master and backport to 2.12.
William

> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Eelco Chaudron Nov. 6, 2019, 7:59 a.m. UTC | #3
On 5 Nov 2019, at 21:54, Ilya Maximets wrote:

> 'xdpmode' is part of 'options', not the 'other_config'.
>
> CC: William Tu <u9012063@gmail.com>
> Fixes: 0de1b425962d ("netdev-afxdp: add new netdev type for AF_XDP.")
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>


LGTM,

Acked-by: Eelco Chaudron <echaudro@redhat.com>
Ilya Maximets Nov. 6, 2019, 12:50 p.m. UTC | #4
On 05.11.2019 22:20, Ben Pfaff wrote:
> On Tue, Nov 05, 2019 at 09:54:08PM +0100, Ilya Maximets wrote:
>> 'xdpmode' is part of 'options', not the 'other_config'.
>>
>> CC: William Tu <u9012063@gmail.com>
>> Fixes: 0de1b425962d ("netdev-afxdp: add new netdev type for AF_XDP.")
>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> 
> It's not clear to me whether the options related to XDP and DPDK and PMD
> consistently follow the intended convention.  The intention is that
> options is for settings that are specific to a particular configured
> interface type, and other-config is for options independent of the
> interface type.

'options'      goes to  netdev_set_config().
'other_config' goes to  dpif_set_port_config().

And that dictates the rules of how we're using them.

'options' are used to store configuration options that needs to be
applied to netdev itself. Those are number of HW queues, different
work modes, numbers of HW descriptors, etc.

'other_config' is only used by dpif-netdev and stores per-port
datapath configurations like enabling/disabling flow caches for
particular port or pinning of netdev rx queues to pmd threads
(this is actually the full list of currently supported configs).

I agree that vswitch.xml might be better structured. For example,
we could make more subcategories for afxdp, dpdk and vhost-user
options to make the list more visual in man pages. It's not fully
clear how to separate 'other_config's. And I'm also not really
comfortable with using "PMD" abbreviation for "netdev that works
in polling mode/interface that requires polling", but I have no
good ideas for short and meaningful replacement.

What we can do is following:

        PMD (Poll Mode Driver) Options:
          options : n_rxq             optional string <...>

          PMD Options: dpdk only:
            options : dpdk-devargs      optional string
            options : n_rxq_desc        optional string, <...>
            options : n_txq_desc        optional string, <...>

          PMD Options: dpdkvhostuserclient only:
            options : vhost-server-path optional string
            options : dq-zero-copy      optional string, <...>
            options : tx-retries-max    optional string, <...>

          PMD Options: afxdp only:
            options : xdpmode           optional string, <...>
            options : use-need-wakeup   optional string, <...>

        Userspace Datapath Configuration:
          other_config : emc-enable   optional string, <...>

          Userspace Datapath Configuration: PMD only:
            other_config : pmd-rxq-affinity  optional string

Suggestions are welcome.

Here we can see that all "PMD" netdevs should somehow support n_rxq
option and it might make sense to pass it via other_config, but this
doesn't fit existing code structure, i.e. will probably have to make
an additional netdev interface like netdev_set_rx_multiq() for passing
that value from the dpif (analogue of existing netdev_set_tx_multiq()).
Another point is that not all the netdevs are "PMD" netdevs, so this
option still kind of netdev specific.


Best regards, Ilya Maximets.
William Tu Nov. 6, 2019, 5:56 p.m. UTC | #5
On Wed, Nov 06, 2019 at 01:50:58PM +0100, Ilya Maximets wrote:
> On 05.11.2019 22:20, Ben Pfaff wrote:
> >On Tue, Nov 05, 2019 at 09:54:08PM +0100, Ilya Maximets wrote:
> >>'xdpmode' is part of 'options', not the 'other_config'.
> >>
> >>CC: William Tu <u9012063@gmail.com>
> >>Fixes: 0de1b425962d ("netdev-afxdp: add new netdev type for AF_XDP.")
> >>Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> >
> >It's not clear to me whether the options related to XDP and DPDK and PMD
> >consistently follow the intended convention.  The intention is that
> >options is for settings that are specific to a particular configured
> >interface type, and other-config is for options independent of the
> >interface type.
> 
> 'options'      goes to  netdev_set_config().
> 'other_config' goes to  dpif_set_port_config().
I think you mean dpif_netdev_set_config

> 
> And that dictates the rules of how we're using them.
> 
> 'options' are used to store configuration options that needs to be
> applied to netdev itself. Those are number of HW queues, different
> work modes, numbers of HW descriptors, etc.
> 
> 'other_config' is only used by dpif-netdev and stores per-port
> datapath configurations like enabling/disabling flow caches for
> particular port or pinning of netdev rx queues to pmd threads
> (this is actually the full list of currently supported configs).
> 
> I agree that vswitch.xml might be better structured. For example,
> we could make more subcategories for afxdp, dpdk and vhost-user
> options to make the list more visual in man pages. It's not fully
> clear how to separate 'other_config's. And I'm also not really
> comfortable with using "PMD" abbreviation for "netdev that works
> in polling mode/interface that requires polling", but I have no

I agree. "PMD" is very confusing, especially "Driver".
How about poll-mode netdev? 

> good ideas for short and meaningful replacement.
> 
> What we can do is following:
> 
>        PMD (Poll Mode Driver) Options:
>          options : n_rxq             optional string <...>
> 
>          PMD Options: dpdk only:
>            options : dpdk-devargs      optional string
>            options : n_rxq_desc        optional string, <...>
>            options : n_txq_desc        optional string, <...>
> 
>          PMD Options: dpdkvhostuserclient only:
>            options : vhost-server-path optional string
>            options : dq-zero-copy      optional string, <...>
>            options : tx-retries-max    optional string, <...>
> 
>          PMD Options: afxdp only:
>            options : xdpmode           optional string, <...>
>            options : use-need-wakeup   optional string, <...>
> 
>        Userspace Datapath Configuration:
>          other_config : emc-enable   optional string, <...>
> 
>          Userspace Datapath Configuration: PMD only:
>            other_config : pmd-rxq-affinity  optional string
> 
> Suggestions are welcome.
> 
> Here we can see that all "PMD" netdevs should somehow support n_rxq
> option and it might make sense to pass it via other_config, but this
> doesn't fit existing code structure, i.e. will probably have to make
> an additional netdev interface like netdev_set_rx_multiq() for passing
> that value from the dpif (analogue of existing netdev_set_tx_multiq()).
> Another point is that not all the netdevs are "PMD" netdevs, so this
> option still kind of netdev specific.
> 
Good idea, I think it's much clearer.

William

> 
> Best regards, Ilya Maximets.
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Ilya Maximets Nov. 6, 2019, 6:20 p.m. UTC | #6
On 06.11.2019 18:56, William Tu wrote:
> On Wed, Nov 06, 2019 at 01:50:58PM +0100, Ilya Maximets wrote:
>> On 05.11.2019 22:20, Ben Pfaff wrote:
>>> On Tue, Nov 05, 2019 at 09:54:08PM +0100, Ilya Maximets wrote:
>>>> 'xdpmode' is part of 'options', not the 'other_config'.
>>>>
>>>> CC: William Tu <u9012063@gmail.com>
>>>> Fixes: 0de1b425962d ("netdev-afxdp: add new netdev type for AF_XDP.")
>>>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
>>>
>>> It's not clear to me whether the options related to XDP and DPDK and PMD
>>> consistently follow the intended convention.  The intention is that
>>> options is for settings that are specific to a particular configured
>>> interface type, and other-config is for options independent of the
>>> interface type.
>>
>> 'options'      goes to  netdev_set_config().
>> 'other_config' goes to  dpif_set_port_config().
> I think you mean dpif_netdev_set_config

Nope, I mean dpif_set_port_config() --> dpif_netdev_port_set_config()
call sequence.

dpif_netdev_set_config() is called from dpif_set_config() and uses
global 'other_config', not the one from 'Interface' table.

> 
>>
>> And that dictates the rules of how we're using them.
>>
>> 'options' are used to store configuration options that needs to be
>> applied to netdev itself. Those are number of HW queues, different
>> work modes, numbers of HW descriptors, etc.
>>
>> 'other_config' is only used by dpif-netdev and stores per-port
>> datapath configurations like enabling/disabling flow caches for
>> particular port or pinning of netdev rx queues to pmd threads
>> (this is actually the full list of currently supported configs).
>>
>> I agree that vswitch.xml might be better structured. For example,
>> we could make more subcategories for afxdp, dpdk and vhost-user
>> options to make the list more visual in man pages. It's not fully
>> clear how to separate 'other_config's. And I'm also not really
>> comfortable with using "PMD" abbreviation for "netdev that works
>> in polling mode/interface that requires polling", but I have no
> 
> I agree. "PMD" is very confusing, especially "Driver".
> How about poll-mode netdev?

Maybe 'poll mode interface' or something like that.
'netdev' is not a user-friendly word.

> 
>> good ideas for short and meaningful replacement.
>>
>> What we can do is following:
>>
>>         PMD (Poll Mode Driver) Options:
>>           options : n_rxq             optional string <...>
>>
>>           PMD Options: dpdk only:
>>             options : dpdk-devargs      optional string
>>             options : n_rxq_desc        optional string, <...>
>>             options : n_txq_desc        optional string, <...>
>>
>>           PMD Options: dpdkvhostuserclient only:
>>             options : vhost-server-path optional string
>>             options : dq-zero-copy      optional string, <...>
>>             options : tx-retries-max    optional string, <...>
>>
>>           PMD Options: afxdp only:
>>             options : xdpmode           optional string, <...>
>>             options : use-need-wakeup   optional string, <...>
>>
>>         Userspace Datapath Configuration:
>>           other_config : emc-enable   optional string, <...>
>>
>>           Userspace Datapath Configuration: PMD only:
>>             other_config : pmd-rxq-affinity  optional string
>>
>> Suggestions are welcome.
>>
>> Here we can see that all "PMD" netdevs should somehow support n_rxq
>> option and it might make sense to pass it via other_config, but this
>> doesn't fit existing code structure, i.e. will probably have to make
>> an additional netdev interface like netdev_set_rx_multiq() for passing
>> that value from the dpif (analogue of existing netdev_set_tx_multiq()).
>> Another point is that not all the netdevs are "PMD" netdevs, so this
>> option still kind of netdev specific.
>>
> Good idea, I think it's much clearer.

You mean restructuration of a man page (vswitch.xml) ?

> 
> William
> 
>>
>> Best regards, Ilya Maximets.
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
William Tu Nov. 6, 2019, 7:12 p.m. UTC | #7
On Wed, Nov 6, 2019 at 10:20 AM Ilya Maximets <i.maximets@ovn.org> wrote:
>
> On 06.11.2019 18:56, William Tu wrote:
> > On Wed, Nov 06, 2019 at 01:50:58PM +0100, Ilya Maximets wrote:
> >> On 05.11.2019 22:20, Ben Pfaff wrote:
> >>> On Tue, Nov 05, 2019 at 09:54:08PM +0100, Ilya Maximets wrote:
> >>>> 'xdpmode' is part of 'options', not the 'other_config'.
> >>>>
> >>>> CC: William Tu <u9012063@gmail.com>
> >>>> Fixes: 0de1b425962d ("netdev-afxdp: add new netdev type for AF_XDP.")
> >>>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> >>>
> >>> It's not clear to me whether the options related to XDP and DPDK and PMD
> >>> consistently follow the intended convention.  The intention is that
> >>> options is for settings that are specific to a particular configured
> >>> interface type, and other-config is for options independent of the
> >>> interface type.
> >>
> >> 'options'      goes to  netdev_set_config().
> >> 'other_config' goes to  dpif_set_port_config().
> > I think you mean dpif_netdev_set_config
>
> Nope, I mean dpif_set_port_config() --> dpif_netdev_port_set_config()
> call sequence.
>
> dpif_netdev_set_config() is called from dpif_set_config() and uses
> global 'other_config', not the one from 'Interface' table.
>

yes, you're right.

> >
> >>
> >> And that dictates the rules of how we're using them.
> >>
> >> 'options' are used to store configuration options that needs to be
> >> applied to netdev itself. Those are number of HW queues, different
> >> work modes, numbers of HW descriptors, etc.
> >>
> >> 'other_config' is only used by dpif-netdev and stores per-port
> >> datapath configurations like enabling/disabling flow caches for
> >> particular port or pinning of netdev rx queues to pmd threads
> >> (this is actually the full list of currently supported configs).
> >>
> >> I agree that vswitch.xml might be better structured. For example,
> >> we could make more subcategories for afxdp, dpdk and vhost-user
> >> options to make the list more visual in man pages. It's not fully
> >> clear how to separate 'other_config's. And I'm also not really
> >> comfortable with using "PMD" abbreviation for "netdev that works
> >> in polling mode/interface that requires polling", but I have no
> >
> > I agree. "PMD" is very confusing, especially "Driver".
> > How about poll-mode netdev?
>
> Maybe 'poll mode interface' or something like that.
> 'netdev' is not a user-friendly word.
>
> >
> >> good ideas for short and meaningful replacement.
> >>
> >> What we can do is following:
> >>
> >>         PMD (Poll Mode Driver) Options:
> >>           options : n_rxq             optional string <...>
> >>
> >>           PMD Options: dpdk only:
> >>             options : dpdk-devargs      optional string
> >>             options : n_rxq_desc        optional string, <...>
> >>             options : n_txq_desc        optional string, <...>
> >>
> >>           PMD Options: dpdkvhostuserclient only:
> >>             options : vhost-server-path optional string
> >>             options : dq-zero-copy      optional string, <...>
> >>             options : tx-retries-max    optional string, <...>
> >>
> >>           PMD Options: afxdp only:
> >>             options : xdpmode           optional string, <...>
> >>             options : use-need-wakeup   optional string, <...>
> >>
> >>         Userspace Datapath Configuration:
> >>           other_config : emc-enable   optional string, <...>
> >>
> >>           Userspace Datapath Configuration: PMD only:
> >>             other_config : pmd-rxq-affinity  optional string
> >>
> >> Suggestions are welcome.
> >>
> >> Here we can see that all "PMD" netdevs should somehow support n_rxq
> >> option and it might make sense to pass it via other_config, but this
> >> doesn't fit existing code structure, i.e. will probably have to make
> >> an additional netdev interface like netdev_set_rx_multiq() for passing
> >> that value from the dpif (analogue of existing netdev_set_tx_multiq()).
> >> Another point is that not all the netdevs are "PMD" netdevs, so this
> >> option still kind of netdev specific.
> >>
> > Good idea, I think it's much clearer.
>
> You mean restructuration of a man page (vswitch.xml) ?
yes, I mean the restructure of man page.

William

Patch
diff mbox series

diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
index 00c6bd2d4..efdfb83bb 100644
--- a/vswitchd/vswitch.xml
+++ b/vswitchd/vswitch.xml
@@ -3107,7 +3107,7 @@  ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \
         </p>
       </column>
 
-      <column name="other_config" key="xdpmode"
+      <column name="options" key="xdpmode"
               type='{"type": "string",
                      "enum": ["set", ["skb", "drv"]]}'>
         <p>