On 5/2/23 15:35, Mike Pattrick wrote: > On Fri, Apr 28, 2023 at 6:46 PM Ilya Maximets <[email protected]> wrote: >> >> On 3/27/23 12:50, Mike Pattrick wrote: >>> From: Flavio Leitner <[email protected]> >>> >>> This patch modifies netdev_get_status to include information about >>> checksum offload status by port, allowing the user to gain insight into >>> where checksum offloading is active. >>> >>> Signed-off-by: Flavio Leitner <[email protected]> >>> Co-authored-by: Mike Pattrick <[email protected]> >>> Signed-off-by: Mike Pattrick <[email protected]> >>> Reviewed-by: David Marchand <[email protected]> >>> --- >>> Since v9: >>> - Removed entire ovs-appctl dpif-netdev/offload-show command, replaced >>> with a field in the netdev status. >>> - Removed duplicative field tx_tso_offload from netdev-dpdk.c >>> Since v10: >>> - No change >>> --- >>> lib/dpif-netdev-unixctl.man | 6 ++++++ >>> lib/netdev-dpdk.c | 5 ----- >>> lib/netdev-provider.h | 1 + >>> lib/netdev.c | 29 ++++++++++++++++++++++++++--- >>> tests/dpif-netdev.at | 18 ++++++++++++++++++ >>> 5 files changed, 51 insertions(+), 8 deletions(-) >>> >>> diff --git a/lib/dpif-netdev-unixctl.man b/lib/dpif-netdev-unixctl.man >>> index 8cd847416..2840d462e 100644 >>> --- a/lib/dpif-netdev-unixctl.man >>> +++ b/lib/dpif-netdev-unixctl.man >>> @@ -262,3 +262,9 @@ PMDs in the case where no value is specified. By >>> default "scalar" is used. >>> \fIstudy_cnt\fR defaults to 128 and indicates the number of packets that >>> the >>> "study" miniflow implementation must parse before choosing an optimal >>> implementation. >>> +. >>> +.IP "\fBdpif-netdev/offload-show\fR [\fIdp\fR] [\fInetdev\fR]" >>> +Prints the hardware offloading features enabled in netdev \fInetdev\fR >>> +attached to datapath \fIdp\fR. The datapath \fIdp\fR parameter can be >>> +omitted if there is only one. All netdev ports are printed if the >>> +parameter \fInetdev\fR is omitted. >> >> We don't have an appctl anymore in this version, so this documentation >> change is now misleading. >> >>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >>> index fb0dd43f7..560694dbc 100644 >>> --- a/lib/netdev-dpdk.c >>> +++ b/lib/netdev-dpdk.c >>> @@ -1753,11 +1753,6 @@ netdev_dpdk_get_config(const struct netdev *netdev, >>> struct smap *args) >>> } else { >>> smap_add(args, "rx_csum_offload", "false"); >>> } >>> - if (dev->hw_ol_features & NETDEV_TX_TSO_OFFLOAD) { >>> - smap_add(args, "tx_tso_offload", "true"); >>> - } else { >>> - smap_add(args, "tx_tso_offload", "false"); >>> - } >>> smap_add(args, "lsc_interrupt_mode", >>> dev->lsc_interrupt_mode ? "true" : "false"); >>> >>> diff --git a/lib/netdev-provider.h b/lib/netdev-provider.h >>> index b5420947d..fcf52bdd9 100644 >>> --- a/lib/netdev-provider.h >>> +++ b/lib/netdev-provider.h >>> @@ -37,6 +37,7 @@ extern "C" { >>> struct netdev_tnl_build_header_params; >>> #define NETDEV_NUMA_UNSPEC OVS_NUMA_UNSPEC >>> >>> +/* Keep this enum updated with translation to string below. */ >>> enum netdev_ol_flags { >>> NETDEV_TX_OFFLOAD_IPV4_CKSUM = 1 << 0, >>> NETDEV_TX_OFFLOAD_TCP_CKSUM = 1 << 1, >>> diff --git a/lib/netdev.c b/lib/netdev.c >>> index c79778378..818589246 100644 >>> --- a/lib/netdev.c >>> +++ b/lib/netdev.c >>> @@ -43,6 +43,7 @@ >>> #include "netdev-provider.h" >>> #include "netdev-vport.h" >>> #include "odp-netlink.h" >>> +#include "openvswitch/json.h" >>> #include "openflow/openflow.h" >>> #include "packets.h" >>> #include "openvswitch/ofp-print.h" >>> @@ -1373,9 +1374,31 @@ netdev_get_next_hop(const struct netdev *netdev, >>> int >>> netdev_get_status(const struct netdev *netdev, struct smap *smap) >>> { >>> - return (netdev->netdev_class->get_status >>> - ? netdev->netdev_class->get_status(netdev, smap) >>> - : EOPNOTSUPP); >>> + int err = EOPNOTSUPP; >>> + >>> + /* Set offload status only if relevant. */ >>> + if (netdev_get_dpif_type(netdev) && >>> + strcmp(netdev_get_dpif_type(netdev), "system")) { >>> + >>> +#define OL_ADD_STAT(name, bit) \ >>> + smap_add(smap, name "_csum_offload", \ >>> + netdev->ol_flags & bit ? "true" : "false"); >>> + >>> + OL_ADD_STAT("ip", NETDEV_TX_OFFLOAD_IPV4_CKSUM); >>> + OL_ADD_STAT("tcp", NETDEV_TX_OFFLOAD_TCP_CKSUM); >>> + OL_ADD_STAT("udp", NETDEV_TX_OFFLOAD_UDP_CKSUM); >>> + OL_ADD_STAT("sctp", NETDEV_TX_OFFLOAD_SCTP_CKSUM); >>> + OL_ADD_STAT("tso", NETDEV_TX_OFFLOAD_TCP_TSO); >> >> The 'tso_csum_offload' name doesn't make a lot of sense. >> 'TCP segmentation offload checksum offload'. It might be better to call >> it tcp_seg_offload. It might also be good to specify the direction, >> because interface may technically support Tx, but not support Rx offload >> for the same thing. >> So, maybe tx-ip-csum-offload, ..., tx-tcp-seg-offload ? >> You had something very similar in v9. >> >> I'm not sure about underscores vs dashes. I'd like dashes, but some other >> status fields are reported with underscores, so I don't know. > > Underscore seems to be the default for status fields as far as I can > tell. I don't have a preference other than consistency. If dashes are > preferable, wouldn't it make sense to homogenize all fields to the > same separator?
I don't have a strong opinion here. I personally like more when use-facing names and options are using dashes. Underscores are less user-friendly. (Database automatically converts one to another in table/column names, for example.) Let's keep upderscores for now. The global change would be out of scope of this patch set. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
