| Message ID | 20191105205408.22071-1-i.maximets@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] vswitch.xml: Fix column for xdpmode. | expand |
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>
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
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>
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.
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
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
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
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>
'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(-)