Re: [ovs-dev] [PATCH v5] netdev: Sync'ed and cleaned {get, set}_config().
On 12.10.23 17:39, Robin Jarry wrote: > Kevin Traynor, Oct 12, 2023 at 17:34: >> ok, 'rss' is documented as default, so maybe we don't need to display if it >> is in use by default, selected by user or as fallback. >> >> That would make things a bit easier as 'rx-steering:' is free to use to >> indicate the unsupported case. >> >> So maybe status could just have: >> 'rx-steering:rss+lacp' for user enabled rss+lacp and NIC supports >> 'rx-steering:unsupported' for user enabled rss+lacp and NIC does not support. >> >> If rx-steering is not reported, then it is using the default 'rss'. >> >> Robin, what do you think ? > > It may be surprising to users not to see any mention in get_config() when > explicitly setting rx-steering=rss but I don't see that as a common/standard > use case. So I think it should be OK. > Thank you Kevin and Robin ☺️ Your change requests have been incorporated in v6: https://patchwork.ozlabs.org/project/openvswitch/list/?series=377463 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v5] netdev: Sync'ed and cleaned {get, set}_config().
Kevin Traynor, Oct 12, 2023 at 17:34: ok, 'rss' is documented as default, so maybe we don't need to display if it is in use by default, selected by user or as fallback. That would make things a bit easier as 'rx-steering:' is free to use to indicate the unsupported case. So maybe status could just have: 'rx-steering:rss+lacp' for user enabled rss+lacp and NIC supports 'rx-steering:unsupported' for user enabled rss+lacp and NIC does not support. If rx-steering is not reported, then it is using the default 'rss'. Robin, what do you think ? It may be surprising to users not to see any mention in get_config() when explicitly setting rx-steering=rss but I don't see that as a common/standard use case. So I think it should be OK. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v5] netdev: Sync'ed and cleaned {get, set}_config().
On 12/10/2023 14:06, Jakob Meng wrote:
On 11.10.23 18:48, Kevin Traynor wrote:
On 11/10/2023 11:11, jm...@redhat.com wrote:
From: Jakob Meng
For better usability, the function pairs get_config() and
set_config() for each netdev 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 moves key-value pairs which are no valid options from
get_config() to the get_status() callback. For example, get_config()
in lib/netdev-dpdk.c returned {configured,requested}_{rx,tx}_queues
previously. For requested rx queues the proper option name is n_rxq,
so requested_rx_queues has been renamed respectively. Tx queues
cannot be changed by the user, hence requested_tx_queues has been
dropped. Both configured_{rx,tx}_queues will be returned as
n_{r,t}xq in the get_status() callback.
vi yo
The documentation in vswitchd/vswitch.xml for status columns as well
as tests have been updated accordingly.
Reported-at: https://bugzilla.redhat.com/1949855
Signed-off-by: Jakob Meng
---
Documentation/intro/install/afxdp.rst | 12 ++---
Documentation/topics/dpdk/phy.rst | 4 +-
lib/netdev-afxdp.c | 21 -
lib/netdev-afxdp.h | 1 +
lib/netdev-dpdk.c | 49 +++-
lib/netdev-dummy.c | 19 ++--
lib/netdev-linux-private.h | 1 +
lib/netdev-linux.c | 4 +-
tests/pmd.at | 26 +--
tests/system-dpdk.at | 64 +--
vswitchd/vswitch.xml | 25 ++-
11 files changed, 150 insertions(+), 76 deletions(-)
Hi Jakob, some initial comments below.
Thank you ☺️
It feels like a lot of changes in one patch, split across different netdevs. I
wonder could it be broken up into patches for the different netdev types ? or
even further like groups for related features, queues/rx-steering/flow control
? Not sure what works best without causing too much intermediate updates with
UTs etc.
Also, I'd suggest adding a cover letter with diff from previous version i.e.
the thing I forgot last week :-)
Ack, will try to split this patch up for the next revision. But first, some
questions below ;)
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/Documentation/topics/dpdk/phy.rst
b/Documentation/topics/dpdk/phy.rst
index f66b106c4..41cc3588a 100644
--- a/Documentation/topics/dpdk/phy.rst
+++ b/Documentation/topics/dpdk/phy.rst
@@ -198,7 +198,7 @@ Example::
a dedicated queue, it will be explicit::
$ ovs-vsctl get interface dpdk-p0 status
- {..., rx_steering=unsupported}
+ {..., rx-steering=unsupported}
The fix is correct, but the meaning of unsupported is changed so text above
(not shown in diff) is incorrect. Mentioning here but I think it will change in
the status reporting and then the docs can be synced with that.
Good catch! We could expose "rx_steer_flows_num" but this would be less self-explanatory
than printing some kind of "unsupported". Any idea how to fix this properly in the docs?
I think the important thing is that user knows the default is rss and
that is mentioned. It could be explicitly stated that it is the default
and only non 'rss' values are shown. Anyway, best to figure out what to
show below and then match the docs to it.
More details can often be found in ``ovs-vswitchd.log``::
@@ -499,7 +499,7 @@ its options::
$ ovs-appctl dpctl/show
[...]
- port 3: dpdk-rep0 (dpdk: configured_rx_queues=1, ...,
dpdk-vf-mac=00:11:22:33:44:55, ...)
+ port 3: dpdk-rep0 (dpdk: ..., dpdk-vf-mac=00:11:22:33:44:55, ...)
$ ovs-vsctl show
[...]
diff --git a/lib/netdev-afxdp.c b/lib/netdev-afxdp.c
index 16f26bc30..8519b5a2b 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", netd
Re: [ovs-dev] [PATCH v5] netdev: Sync'ed and cleaned {get, set}_config().
On 11.10.23 18:48, Kevin Traynor wrote:
> On 11/10/2023 11:11, jm...@redhat.com wrote:
>> From: Jakob Meng
>>
>> For better usability, the function pairs get_config() and
>> set_config() for each netdev 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 moves key-value pairs which are no valid options from
>> get_config() to the get_status() callback. For example, get_config()
>> in lib/netdev-dpdk.c returned {configured,requested}_{rx,tx}_queues
>> previously. For requested rx queues the proper option name is n_rxq,
>> so requested_rx_queues has been renamed respectively. Tx queues
>> cannot be changed by the user, hence requested_tx_queues has been
>> dropped. Both configured_{rx,tx}_queues will be returned as
>> n_{r,t}xq in the get_status() callback.
>> vi yo
>
>> The documentation in vswitchd/vswitch.xml for status columns as well
>> as tests have been updated accordingly.
>>
>> Reported-at: https://bugzilla.redhat.com/1949855
>> Signed-off-by: Jakob Meng
>> ---
>> Documentation/intro/install/afxdp.rst | 12 ++---
>> Documentation/topics/dpdk/phy.rst | 4 +-
>> lib/netdev-afxdp.c | 21 -
>> lib/netdev-afxdp.h | 1 +
>> lib/netdev-dpdk.c | 49 +++-
>> lib/netdev-dummy.c | 19 ++--
>> lib/netdev-linux-private.h | 1 +
>> lib/netdev-linux.c | 4 +-
>> tests/pmd.at | 26 +--
>> tests/system-dpdk.at | 64 +--
>> vswitchd/vswitch.xml | 25 ++-
>> 11 files changed, 150 insertions(+), 76 deletions(-)
>>
>
> Hi Jakob, some initial comments below.
>
Thank you ☺️
> It feels like a lot of changes in one patch, split across different netdevs.
> I wonder could it be broken up into patches for the different netdev types ?
> or even further like groups for related features, queues/rx-steering/flow
> control ? Not sure what works best without causing too much intermediate
> updates with UTs etc.
>
> Also, I'd suggest adding a cover letter with diff from previous version i.e.
> the thing I forgot last week :-)
>
Ack, will try to split this patch up for the next revision. But first, some
questions below ;)
>
>> 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/Documentation/topics/dpdk/phy.rst
>> b/Documentation/topics/dpdk/phy.rst
>> index f66b106c4..41cc3588a 100644
>> --- a/Documentation/topics/dpdk/phy.rst
>> +++ b/Documentation/topics/dpdk/phy.rst
>> @@ -198,7 +198,7 @@ Example::
>> a dedicated queue, it will be explicit::
>> $ ovs-vsctl get interface dpdk-p0 status
>> - {..., rx_steering=unsupported}
>> + {..., rx-steering=unsupported}
>>
>
> The fix is correct, but the meaning of unsupported is changed so text above
> (not shown in diff) is incorrect. Mentioning here but I think it will change
> in the status reporting and then the docs can be synced with that.
Good catch! We could expose "rx_steer_flows_num" but this would be less
self-explanatory than printing some kind of "unsupported". Any idea how to fix
this properly in the docs?
>
>> More details can often be found in ``ovs-vswitchd.log``::
>> @@ -499,7 +499,7 @@ its options::
>> $ ovs-appctl dpctl/show
>> [...]
>> - port 3: dpdk-rep0 (dpdk: configured_rx_queues=1, ...,
>> dpdk-vf-mac=00:11:22:33:44:55, ...)
>> + port 3: dpdk-rep0 (dpdk: ..., dpdk-vf-mac=00:11:22:33:44:55, ...)
>> $ ovs-vsctl show
>> [...]
>> diff --git a/lib/netdev-afxdp.c b/lib/netdev-afxdp.c
>> index 16f26bc30..8519b5a2b 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",
Re: [ovs-dev] [PATCH v5] netdev: Sync'ed and cleaned {get, set}_config().
On 11/10/2023 11:11, jm...@redhat.com wrote:
From: Jakob Meng
For better usability, the function pairs get_config() and
set_config() for each netdev 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 moves key-value pairs which are no valid options from
get_config() to the get_status() callback. For example, get_config()
in lib/netdev-dpdk.c returned {configured,requested}_{rx,tx}_queues
previously. For requested rx queues the proper option name is n_rxq,
so requested_rx_queues has been renamed respectively. Tx queues
cannot be changed by the user, hence requested_tx_queues has been
dropped. Both configured_{rx,tx}_queues will be returned as
n_{r,t}xq in the get_status() callback.
vi yo
The documentation in vswitchd/vswitch.xml for status columns as well
as tests have been updated accordingly.
Reported-at: https://bugzilla.redhat.com/1949855
Signed-off-by: Jakob Meng
---
Documentation/intro/install/afxdp.rst | 12 ++---
Documentation/topics/dpdk/phy.rst | 4 +-
lib/netdev-afxdp.c| 21 -
lib/netdev-afxdp.h| 1 +
lib/netdev-dpdk.c | 49 +++-
lib/netdev-dummy.c| 19 ++--
lib/netdev-linux-private.h| 1 +
lib/netdev-linux.c| 4 +-
tests/pmd.at | 26 +--
tests/system-dpdk.at | 64 +--
vswitchd/vswitch.xml | 25 ++-
11 files changed, 150 insertions(+), 76 deletions(-)
Hi Jakob, some initial comments below.
It feels like a lot of changes in one patch, split across different
netdevs. I wonder could it be broken up into patches for the different
netdev types ? or even further like groups for related features,
queues/rx-steering/flow control ? Not sure what works best without
causing too much intermediate updates with UTs etc.
Also, I'd suggest adding a cover letter with diff from previous version
i.e. the thing I forgot last week :-)
thanks,
Kevin.
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/Documentation/topics/dpdk/phy.rst
b/Documentation/topics/dpdk/phy.rst
index f66b106c4..41cc3588a 100644
--- a/Documentation/topics/dpdk/phy.rst
+++ b/Documentation/topics/dpdk/phy.rst
@@ -198,7 +198,7 @@ Example::
a dedicated queue, it will be explicit::
$ ovs-vsctl get interface dpdk-p0 status
- {..., rx_steering=unsupported}
+ {..., rx-steering=unsupported}
The fix is correct, but the meaning of unsupported is changed so text
above (not shown in diff) is incorrect. Mentioning here but I think it
will change in the status reporting and then the docs can be synced with
that.
More details can often be found in ``ovs-vswitchd.log``::
@@ -499,7 +499,7 @@ its options::
$ ovs-appctl dpctl/show
[...]
- port 3: dpdk-rep0 (dpdk: configured_rx_queues=1, ...,
dpdk-vf-mac=00:11:22:33:44:55, ...)
+ port 3: dpdk-rep0 (dpdk: ..., dpdk-vf-mac=00:11:22:33:44:55, ...)
$ ovs-vsctl show
[...]
diff --git a/lib/netdev-afxdp.c b/lib/netdev-afxdp.c
index 16f26bc30..8519b5a2b 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);
blank line he
[ovs-dev] [PATCH v5] netdev: Sync'ed and cleaned {get, set}_config().
From: Jakob Meng
For better usability, the function pairs get_config() and
set_config() for each netdev 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 moves key-value pairs which are no valid options from
get_config() to the get_status() callback. For example, get_config()
in lib/netdev-dpdk.c returned {configured,requested}_{rx,tx}_queues
previously. For requested rx queues the proper option name is n_rxq,
so requested_rx_queues has been renamed respectively. Tx queues
cannot be changed by the user, hence requested_tx_queues has been
dropped. Both configured_{rx,tx}_queues will be returned as
n_{r,t}xq in the get_status() callback.
The documentation in vswitchd/vswitch.xml for status columns as well
as tests have been updated accordingly.
Reported-at: https://bugzilla.redhat.com/1949855
Signed-off-by: Jakob Meng
---
Documentation/intro/install/afxdp.rst | 12 ++---
Documentation/topics/dpdk/phy.rst | 4 +-
lib/netdev-afxdp.c| 21 -
lib/netdev-afxdp.h| 1 +
lib/netdev-dpdk.c | 49 +++-
lib/netdev-dummy.c| 19 ++--
lib/netdev-linux-private.h| 1 +
lib/netdev-linux.c| 4 +-
tests/pmd.at | 26 +--
tests/system-dpdk.at | 64 +--
vswitchd/vswitch.xml | 25 ++-
11 files changed, 150 insertions(+), 76 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/Documentation/topics/dpdk/phy.rst
b/Documentation/topics/dpdk/phy.rst
index f66b106c4..41cc3588a 100644
--- a/Documentation/topics/dpdk/phy.rst
+++ b/Documentation/topics/dpdk/phy.rst
@@ -198,7 +198,7 @@ Example::
a dedicated queue, it will be explicit::
$ ovs-vsctl get interface dpdk-p0 status
- {..., rx_steering=unsupported}
+ {..., rx-steering=unsupported}
More details can often be found in ``ovs-vswitchd.log``::
@@ -499,7 +499,7 @@ its options::
$ ovs-appctl dpctl/show
[...]
- port 3: dpdk-rep0 (dpdk: configured_rx_queues=1, ...,
dpdk-vf-mac=00:11:22:33:44:55, ...)
+ port 3: dpdk-rep0 (dpdk: ..., dpdk-vf-mac=00:11:22:33:44:55, ...)
$ ovs-vsctl show
[...]
diff --git a/lib/netdev-afxdp.c b/lib/netdev-afxdp.c
index 16f26bc30..8519b5a2b 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
