On 10/30/23 10:49, jm...@redhat.com wrote:
> From: Jakob Meng <c...@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 <c...@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">

_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to