| Message ID | 20231030095000.720854-3-jmeng@redhat.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | netdev: Sync and clean {get, set}_config() callbacks. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On 30/10/2023 09:49, jmeng@redhat.com wrote: > From: Jakob Meng<code@jakobmeng.de> > > For better usability, the function pairs get_config() and > set_config() for netdevs should be symmetric: Options which are > accepted by set_config() should be returned by get_config() and the > latter should output valid options for set_config() only. This patch > also moves key-value pairs which are no valid options from get_config() > to the get_status() callback. > > The documentation in vswitchd/vswitch.xml for status columns has been > updated accordingly. > > Reported-at:https://bugzilla.redhat.com/1949855 > Signed-off-by: Jakob Meng<code@jakobmeng.de> > --- > Documentation/intro/install/afxdp.rst | 12 ++++-------- > lib/netdev-afxdp.c | 21 +++++++++++++++++++-- > lib/netdev-afxdp.h | 1 + > lib/netdev-linux-private.h | 1 + > lib/netdev-linux.c | 4 ++-- > vswitchd/vswitch.xml | 11 +++++++++++ > 6 files changed, 38 insertions(+), 12 deletions(-) This needs a small rebase for vswitch.xml due to vhost additions in other patch series, but no need to send a new version for that, it can handled when pushing. Acked-by: Kevin Traynor <ktraynor@redhat.com>
On 10/30/23 10:49, jmeng@redhat.com wrote: > From: Jakob Meng <code@jakobmeng.de> > > For better usability, the function pairs get_config() and > set_config() for netdevs should be symmetric: Options which are > accepted by set_config() should be returned by get_config() and the > latter should output valid options for set_config() only. This patch > also moves key-value pairs which are no valid options from get_config() > to the get_status() callback. > > The documentation in vswitchd/vswitch.xml for status columns has been > updated accordingly. > > Reported-at: https://bugzilla.redhat.com/1949855 > Signed-off-by: Jakob Meng <code@jakobmeng.de> > --- > Documentation/intro/install/afxdp.rst | 12 ++++-------- > lib/netdev-afxdp.c | 21 +++++++++++++++++++-- > lib/netdev-afxdp.h | 1 + > lib/netdev-linux-private.h | 1 + > lib/netdev-linux.c | 4 ++-- > vswitchd/vswitch.xml | 11 +++++++++++ > 6 files changed, 38 insertions(+), 12 deletions(-) > > diff --git a/Documentation/intro/install/afxdp.rst b/Documentation/intro/install/afxdp.rst > index 51c24bf5b..5776614c8 100644 > --- a/Documentation/intro/install/afxdp.rst > +++ b/Documentation/intro/install/afxdp.rst > @@ -219,14 +219,10 @@ Otherwise, enable debugging by:: > ovs-appctl vlog/set netdev_afxdp::dbg > > To check which XDP mode was chosen by ``best-effort``, you can look for > -``xdp-mode-in-use`` in the output of ``ovs-appctl dpctl/show``:: > - > - # ovs-appctl dpctl/show > - netdev@ovs-netdev: > - <...> > - port 2: ens802f0 (afxdp: n_rxq=1, use-need-wakeup=true, > - xdp-mode=best-effort, > - xdp-mode-in-use=native-with-zerocopy) > +``xdp-mode`` in the output of ``ovs-vsctl get interface INT status:xdp-mode``:: > + > + # ovs-vsctl get interface ens802f0 status:xdp-mode > + "native-with-zerocopy" > > References > ---------- > diff --git a/lib/netdev-afxdp.c b/lib/netdev-afxdp.c > index 16f26bc30..b680a1479 100644 > --- a/lib/netdev-afxdp.c > +++ b/lib/netdev-afxdp.c > @@ -672,8 +672,6 @@ netdev_afxdp_get_config(const struct netdev *netdev, struct smap *args) > ovs_mutex_lock(&dev->mutex); > smap_add_format(args, "n_rxq", "%d", netdev->n_rxq); > smap_add_format(args, "xdp-mode", "%s", xdp_modes[dev->xdp_mode].name); > - smap_add_format(args, "xdp-mode-in-use", "%s", > - xdp_modes[dev->xdp_mode_in_use].name); > smap_add_format(args, "use-need-wakeup", "%s", > dev->use_need_wakeup ? "true" : "false"); > ovs_mutex_unlock(&dev->mutex); > @@ -1367,3 +1365,22 @@ netdev_afxdp_get_stats(const struct netdev *netdev, > > return error; > } > + > +int > +netdev_afxdp_get_status(const struct netdev *netdev, struct smap *args) > +{ > + int error = netdev_linux_get_status(netdev, args); > + > + if (error) { > + return error; > + } > + > + struct netdev_linux *dev = netdev_linux_cast(netdev); > + > + ovs_mutex_lock(&dev->mutex); > + smap_add_format(args, "xdp-mode", "%s", > + xdp_modes[dev->xdp_mode_in_use].name); > + ovs_mutex_unlock(&dev->mutex); > + > + return error; > +} > diff --git a/lib/netdev-afxdp.h b/lib/netdev-afxdp.h > index e91cd102d..bd3b9dfbe 100644 > --- a/lib/netdev-afxdp.h > +++ b/lib/netdev-afxdp.h > @@ -63,6 +63,7 @@ int netdev_afxdp_set_config(struct netdev *netdev, const struct smap *args, > int netdev_afxdp_get_config(const struct netdev *netdev, struct smap *args); > int netdev_afxdp_get_stats(const struct netdev *netdev_, > struct netdev_stats *stats); > +int netdev_afxdp_get_status(const struct netdev *netdev, struct smap *args); > int netdev_afxdp_get_custom_stats(const struct netdev *netdev, > struct netdev_custom_stats *custom_stats); > > diff --git a/lib/netdev-linux-private.h b/lib/netdev-linux-private.h > index 0ecf0f748..188e8438a 100644 > --- a/lib/netdev-linux-private.h > +++ b/lib/netdev-linux-private.h > @@ -50,6 +50,7 @@ struct netdev_rxq_linux { > }; > > int netdev_linux_construct(struct netdev *); > +int netdev_linux_get_status(const struct netdev *, struct smap *); > void netdev_linux_run(const struct netdev_class *); > > int get_stats_via_netlink(const struct netdev *netdev_, > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c > index cca340879..70521e3c7 100644 > --- a/lib/netdev-linux.c > +++ b/lib/netdev-linux.c > @@ -3493,7 +3493,7 @@ netdev_linux_get_next_hop(const struct in_addr *host, struct in_addr *next_hop, > return ENXIO; > } > > -static int > +int > netdev_linux_get_status(const struct netdev *netdev_, struct smap *smap) > { > struct netdev_linux *netdev = netdev_linux_cast(netdev_); > @@ -3759,7 +3759,7 @@ const struct netdev_class netdev_internal_class = { > .destruct = netdev_afxdp_destruct, \ > .get_stats = netdev_afxdp_get_stats, \ > .get_custom_stats = netdev_afxdp_get_custom_stats, \ > - .get_status = netdev_linux_get_status, \ > + .get_status = netdev_afxdp_get_status, \ > .set_config = netdev_afxdp_set_config, \ > .get_config = netdev_afxdp_get_config, \ > .reconfigure = netdev_afxdp_reconfigure, \ > diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml > index 1e2a1267d..d84260d75 100644 > --- a/vswitchd/vswitch.xml > +++ b/vswitchd/vswitch.xml > @@ -3821,6 +3821,17 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \ > supported by hardware. > </column> > </group> > + > + <group title="afxdp"> > + <p> > + AF_XDP netdev specific interface status options. 'netdev' word seems redundant here. 'netdev' and 'interface' are basically words that describe the same thing in this context. And the 'interface' should be preferred in the user documentation, as that is the entity users actually interact with via database. > + </p> > + > + <column name="status" key="xdp-mode"> > + XDP mode which was chosen. The "was chosen" here is a bit ambiguous as it may be interpreted as both "chosen by the user" and "chosen by OVS itself". "currently in use" might be a better way to describe the meaning, as it kind of was before. It might also be useful to reference the <ref column="options" key="xdp-mode" /> for the description of the possible values. > + </column> > + > + </group> > </group> > > <group title="Statistics">
On 03.11.23 22:44, Ilya Maximets wrote: > On 10/30/23 10:49, jmeng@redhat.com wrote: >> From: Jakob Meng <code@jakobmeng.de> >> >> For better usability, the function pairs get_config() and >> set_config() for netdevs should be symmetric: Options which are >> accepted by set_config() should be returned by get_config() and the >> latter should output valid options for set_config() only. This patch >> also moves key-value pairs which are no valid options from get_config() >> to the get_status() callback. >> >> ... >> >> --- a/vswitchd/vswitch.xml >> +++ b/vswitchd/vswitch.xml >> @@ -3821,6 +3821,17 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \ >> supported by hardware. >> </column> >> </group> >> + >> + <group title="afxdp"> >> + <p> >> + AF_XDP netdev specific interface status options. > 'netdev' word seems redundant here. 'netdev' and 'interface' are > basically words that describe the same thing in this context. > And the 'interface' should be preferred in the user documentation, > as that is the entity users actually interact with via database. > >> + </p> >> + >> + <column name="status" key="xdp-mode"> >> + XDP mode which was chosen. > The "was chosen" here is a bit ambiguous as it may be interpreted > as both "chosen by the user" and "chosen by OVS itself". > "currently in use" might be a better way to describe the meaning, > as it kind of was before. > > It might also be useful to reference the <ref column="options" > key="xdp-mode" /> for the description of the possible values. > >> + </column> >> + >> + </group> >> </group> >> >> <group title="Statistics"> ack, ty, integrated into patch series v8: https://patchwork.ozlabs.org/project/openvswitch/list/?series=381909
diff --git a/Documentation/intro/install/afxdp.rst b/Documentation/intro/install/afxdp.rst index 51c24bf5b..5776614c8 100644 --- a/Documentation/intro/install/afxdp.rst +++ b/Documentation/intro/install/afxdp.rst @@ -219,14 +219,10 @@ Otherwise, enable debugging by:: ovs-appctl vlog/set netdev_afxdp::dbg To check which XDP mode was chosen by ``best-effort``, you can look for -``xdp-mode-in-use`` in the output of ``ovs-appctl dpctl/show``:: - - # ovs-appctl dpctl/show - netdev@ovs-netdev: - <...> - port 2: ens802f0 (afxdp: n_rxq=1, use-need-wakeup=true, - xdp-mode=best-effort, - xdp-mode-in-use=native-with-zerocopy) +``xdp-mode`` in the output of ``ovs-vsctl get interface INT status:xdp-mode``:: + + # ovs-vsctl get interface ens802f0 status:xdp-mode + "native-with-zerocopy" References ---------- diff --git a/lib/netdev-afxdp.c b/lib/netdev-afxdp.c index 16f26bc30..b680a1479 100644 --- a/lib/netdev-afxdp.c +++ b/lib/netdev-afxdp.c @@ -672,8 +672,6 @@ netdev_afxdp_get_config(const struct netdev *netdev, struct smap *args) ovs_mutex_lock(&dev->mutex); smap_add_format(args, "n_rxq", "%d", netdev->n_rxq); smap_add_format(args, "xdp-mode", "%s", xdp_modes[dev->xdp_mode].name); - smap_add_format(args, "xdp-mode-in-use", "%s", - xdp_modes[dev->xdp_mode_in_use].name); smap_add_format(args, "use-need-wakeup", "%s", dev->use_need_wakeup ? "true" : "false"); ovs_mutex_unlock(&dev->mutex); @@ -1367,3 +1365,22 @@ netdev_afxdp_get_stats(const struct netdev *netdev, return error; } + +int +netdev_afxdp_get_status(const struct netdev *netdev, struct smap *args) +{ + int error = netdev_linux_get_status(netdev, args); + + if (error) { + return error; + } + + struct netdev_linux *dev = netdev_linux_cast(netdev); + + ovs_mutex_lock(&dev->mutex); + smap_add_format(args, "xdp-mode", "%s", + xdp_modes[dev->xdp_mode_in_use].name); + ovs_mutex_unlock(&dev->mutex); + + return error; +} diff --git a/lib/netdev-afxdp.h b/lib/netdev-afxdp.h index e91cd102d..bd3b9dfbe 100644 --- a/lib/netdev-afxdp.h +++ b/lib/netdev-afxdp.h @@ -63,6 +63,7 @@ int netdev_afxdp_set_config(struct netdev *netdev, const struct smap *args, int netdev_afxdp_get_config(const struct netdev *netdev, struct smap *args); int netdev_afxdp_get_stats(const struct netdev *netdev_, struct netdev_stats *stats); +int netdev_afxdp_get_status(const struct netdev *netdev, struct smap *args); int netdev_afxdp_get_custom_stats(const struct netdev *netdev, struct netdev_custom_stats *custom_stats); diff --git a/lib/netdev-linux-private.h b/lib/netdev-linux-private.h index 0ecf0f748..188e8438a 100644 --- a/lib/netdev-linux-private.h +++ b/lib/netdev-linux-private.h @@ -50,6 +50,7 @@ struct netdev_rxq_linux { }; int netdev_linux_construct(struct netdev *); +int netdev_linux_get_status(const struct netdev *, struct smap *); void netdev_linux_run(const struct netdev_class *); int get_stats_via_netlink(const struct netdev *netdev_, diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c index cca340879..70521e3c7 100644 --- a/lib/netdev-linux.c +++ b/lib/netdev-linux.c @@ -3493,7 +3493,7 @@ netdev_linux_get_next_hop(const struct in_addr *host, struct in_addr *next_hop, return ENXIO; } -static int +int netdev_linux_get_status(const struct netdev *netdev_, struct smap *smap) { struct netdev_linux *netdev = netdev_linux_cast(netdev_); @@ -3759,7 +3759,7 @@ const struct netdev_class netdev_internal_class = { .destruct = netdev_afxdp_destruct, \ .get_stats = netdev_afxdp_get_stats, \ .get_custom_stats = netdev_afxdp_get_custom_stats, \ - .get_status = netdev_linux_get_status, \ + .get_status = netdev_afxdp_get_status, \ .set_config = netdev_afxdp_set_config, \ .get_config = netdev_afxdp_get_config, \ .reconfigure = netdev_afxdp_reconfigure, \ diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml index 1e2a1267d..d84260d75 100644 --- a/vswitchd/vswitch.xml +++ b/vswitchd/vswitch.xml @@ -3821,6 +3821,17 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \ supported by hardware. </column> </group> + + <group title="afxdp"> + <p> + AF_XDP netdev specific interface status options. + </p> + + <column name="status" key="xdp-mode"> + XDP mode which was chosen. + </column> + + </group> </group> <group title="Statistics">