| Message ID | 20231030065838.2083379-1-mkp@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v5,1/3] netdev-linux: Use ethtool to detect offload support. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/intel-ovs-compilation | success | test: success |
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On Mon, Oct 30, 2023 at 02:58:36AM -0400, Mike Pattrick wrote: > Currently when userspace-tso is enabled, netdev-linux interfaces will > indicate support for all offload flags regardless of interface > configuration. This patch checks for which offload features are enabled > during netdev construction. > > Signed-off-by: Mike Pattrick <mkp@redhat.com> ... > @@ -2381,6 +2376,153 @@ netdev_internal_get_stats(const struct netdev *netdev_, > return error; > } > > +static int > +netdev_linux_read_stringset_info(struct netdev_linux *netdev, uint32_t *len) > +{ > + /* > + struct { > + union { > + struct ethtool_sset_info hdr; > + struct { > + uint64_t pad[2]; > + uint32_t sset_len[1]; > + }; > + }; > + } sset_info; > + */ Hi Mike, maybe the comment above is an artifact that should be removed? > + union { > + struct ethtool_sset_info hdr; > + struct { > + uint64_t pad[2]; > + uint32_t sset_len[1]; > + }; > + } sset_info; > + > + sset_info.hdr.cmd = ETHTOOL_GSSET_INFO; > + sset_info.hdr.reserved = 0; > + sset_info.hdr.sset_mask = 1ULL << ETH_SS_FEATURES; > + > + int error = netdev_linux_do_ethtool(netdev->up.name, > + (struct ethtool_cmd *)&sset_info, > + ETHTOOL_GSSET_INFO, "ETHTOOL_GSSET_INFO"); > + if (error) { > + return error; > + } > + *len = sset_info.sset_len[0]; > + return 0; > +} > + > + > +static int > +netdev_linux_read_definitions(struct netdev_linux *netdev, > + struct ethtool_gstrings **pstrings) > +{ > + int error = 0; > + struct ethtool_gstrings *strings = NULL; > + uint32_t len = 0; Reverse xmas tree please. > + > + error = netdev_linux_read_stringset_info(netdev, &len); > + if (error || !len) { > + return error; > + } > + strings = xcalloc(1, sizeof(*strings) + len * ETH_GSTRING_LEN); > + if (!strings) { > + return ENOMEM; > + } > + > + strings->cmd = ETHTOOL_GSTRINGS; > + strings->string_set = ETH_SS_FEATURES; > + strings->len = len; > + error = netdev_linux_do_ethtool(netdev->up.name, > + (struct ethtool_cmd *) strings, > + ETHTOOL_GSTRINGS, "ETHTOOL_GSTRINGS"); > + if (error) { > + goto out; > + } > + > + for (int i = 0; i < len; i++) { > + strings->data[(i + 1) * ETH_GSTRING_LEN - 1] = 0; > + } > + > + *pstrings = strings; > + > + return 0; > +out: > + *pstrings = NULL; > + free(strings); > + return error; > +} > + > +static void > +netdev_linux_set_ol(struct netdev *netdev_) > +{ > + struct netdev_linux *netdev = netdev_linux_cast(netdev_); > + struct ethtool_gstrings *names = NULL; > + struct ethtool_gfeatures *features = NULL; > + int error; Ditto. ...
On Mon, Oct 30, 2023 at 7:43 AM Simon Horman <horms@ovn.org> wrote: > > On Mon, Oct 30, 2023 at 02:58:36AM -0400, Mike Pattrick wrote: > > Currently when userspace-tso is enabled, netdev-linux interfaces will > > indicate support for all offload flags regardless of interface > > configuration. This patch checks for which offload features are enabled > > during netdev construction. > > > > Signed-off-by: Mike Pattrick <mkp@redhat.com> > > ... > > > @@ -2381,6 +2376,153 @@ netdev_internal_get_stats(const struct netdev *netdev_, > > return error; > > } > > > > +static int > > +netdev_linux_read_stringset_info(struct netdev_linux *netdev, uint32_t *len) > > +{ > > + /* > > + struct { > > + union { > > + struct ethtool_sset_info hdr; > > + struct { > > + uint64_t pad[2]; > > + uint32_t sset_len[1]; > > + }; > > + }; > > + } sset_info; > > + */ > > Hi Mike, > > maybe the comment above is an artifact that should be removed? Hello Simon, Yes, that was some code mistakenly left in, sorry about that. Do you want me to resubmit the whole series now, or were you planning on taking a look at other patches in the series? Thanks for the review, Mike > > > + union { > > + struct ethtool_sset_info hdr; > > + struct { > > + uint64_t pad[2]; > > + uint32_t sset_len[1]; > > + }; > > + } sset_info; > > + > > + sset_info.hdr.cmd = ETHTOOL_GSSET_INFO; > > + sset_info.hdr.reserved = 0; > > + sset_info.hdr.sset_mask = 1ULL << ETH_SS_FEATURES; > > + > > + int error = netdev_linux_do_ethtool(netdev->up.name, > > + (struct ethtool_cmd *)&sset_info, > > + ETHTOOL_GSSET_INFO, "ETHTOOL_GSSET_INFO"); > > + if (error) { > > + return error; > > + } > > + *len = sset_info.sset_len[0]; > > + return 0; > > +} > > + > > + > > +static int > > +netdev_linux_read_definitions(struct netdev_linux *netdev, > > + struct ethtool_gstrings **pstrings) > > +{ > > + int error = 0; > > + struct ethtool_gstrings *strings = NULL; > > + uint32_t len = 0; > > Reverse xmas tree please. > > > + > > + error = netdev_linux_read_stringset_info(netdev, &len); > > + if (error || !len) { > > + return error; > > + } > > + strings = xcalloc(1, sizeof(*strings) + len * ETH_GSTRING_LEN); > > + if (!strings) { > > + return ENOMEM; > > + } > > + > > + strings->cmd = ETHTOOL_GSTRINGS; > > + strings->string_set = ETH_SS_FEATURES; > > + strings->len = len; > > + error = netdev_linux_do_ethtool(netdev->up.name, > > + (struct ethtool_cmd *) strings, > > + ETHTOOL_GSTRINGS, "ETHTOOL_GSTRINGS"); > > + if (error) { > > + goto out; > > + } > > + > > + for (int i = 0; i < len; i++) { > > + strings->data[(i + 1) * ETH_GSTRING_LEN - 1] = 0; > > + } > > + > > + *pstrings = strings; > > + > > + return 0; > > +out: > > + *pstrings = NULL; > > + free(strings); > > + return error; > > +} > > + > > +static void > > +netdev_linux_set_ol(struct netdev *netdev_) > > +{ > > + struct netdev_linux *netdev = netdev_linux_cast(netdev_); > > + struct ethtool_gstrings *names = NULL; > > + struct ethtool_gfeatures *features = NULL; > > + int error; > > Ditto. > > ... >
On Tue, Oct 31, 2023 at 10:29:47AM -0400, Mike Pattrick wrote: > On Mon, Oct 30, 2023 at 7:43 AM Simon Horman <horms@ovn.org> wrote: > > > > On Mon, Oct 30, 2023 at 02:58:36AM -0400, Mike Pattrick wrote: > > > Currently when userspace-tso is enabled, netdev-linux interfaces will > > > indicate support for all offload flags regardless of interface > > > configuration. This patch checks for which offload features are enabled > > > during netdev construction. > > > > > > Signed-off-by: Mike Pattrick <mkp@redhat.com> > > > > ... > > > > > @@ -2381,6 +2376,153 @@ netdev_internal_get_stats(const struct netdev *netdev_, > > > return error; > > > } > > > > > > +static int > > > +netdev_linux_read_stringset_info(struct netdev_linux *netdev, uint32_t *len) > > > +{ > > > + /* > > > + struct { > > > + union { > > > + struct ethtool_sset_info hdr; > > > + struct { > > > + uint64_t pad[2]; > > > + uint32_t sset_len[1]; > > > + }; > > > + }; > > > + } sset_info; > > > + */ > > > > Hi Mike, > > > > maybe the comment above is an artifact that should be removed? > > Hello Simon, > > Yes, that was some code mistakenly left in, sorry about that. > > Do you want me to resubmit the whole series now, or were you planning > on taking a look at other patches in the series? Hi Mike, I think resubmitting now would be a good idea.
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c index cca340879..d1482178b 100644 --- a/lib/netdev-linux.c +++ b/lib/netdev-linux.c @@ -558,6 +558,7 @@ static bool netdev_linux_miimon_enabled(void); static void netdev_linux_miimon_run(void); static void netdev_linux_miimon_wait(void); static int netdev_linux_get_mtu__(struct netdev_linux *netdev, int *mtup); +static void netdev_linux_set_ol(struct netdev *netdev); static bool is_tap_netdev(const struct netdev *netdev) @@ -959,14 +960,8 @@ netdev_linux_construct(struct netdev *netdev_) return error; } - /* The socket interface doesn't offer the option to enable only - * csum offloading without TSO. */ if (userspace_tso_enabled()) { - netdev_->ol_flags |= NETDEV_TX_OFFLOAD_TCP_TSO; - netdev_->ol_flags |= NETDEV_TX_OFFLOAD_TCP_CKSUM; - netdev_->ol_flags |= NETDEV_TX_OFFLOAD_UDP_CKSUM; - netdev_->ol_flags |= NETDEV_TX_OFFLOAD_SCTP_CKSUM; - netdev_->ol_flags |= NETDEV_TX_OFFLOAD_IPV4_CKSUM; + netdev_linux_set_ol(netdev_); } error = get_flags(&netdev->up, &netdev->ifi_flags); @@ -2381,6 +2376,153 @@ netdev_internal_get_stats(const struct netdev *netdev_, return error; } +static int +netdev_linux_read_stringset_info(struct netdev_linux *netdev, uint32_t *len) +{ + /* + struct { + union { + struct ethtool_sset_info hdr; + struct { + uint64_t pad[2]; + uint32_t sset_len[1]; + }; + }; + } sset_info; + */ + union { + struct ethtool_sset_info hdr; + struct { + uint64_t pad[2]; + uint32_t sset_len[1]; + }; + } sset_info; + + sset_info.hdr.cmd = ETHTOOL_GSSET_INFO; + sset_info.hdr.reserved = 0; + sset_info.hdr.sset_mask = 1ULL << ETH_SS_FEATURES; + + int error = netdev_linux_do_ethtool(netdev->up.name, + (struct ethtool_cmd *)&sset_info, + ETHTOOL_GSSET_INFO, "ETHTOOL_GSSET_INFO"); + if (error) { + return error; + } + *len = sset_info.sset_len[0]; + return 0; +} + + +static int +netdev_linux_read_definitions(struct netdev_linux *netdev, + struct ethtool_gstrings **pstrings) +{ + int error = 0; + struct ethtool_gstrings *strings = NULL; + uint32_t len = 0; + + error = netdev_linux_read_stringset_info(netdev, &len); + if (error || !len) { + return error; + } + strings = xcalloc(1, sizeof(*strings) + len * ETH_GSTRING_LEN); + if (!strings) { + return ENOMEM; + } + + strings->cmd = ETHTOOL_GSTRINGS; + strings->string_set = ETH_SS_FEATURES; + strings->len = len; + error = netdev_linux_do_ethtool(netdev->up.name, + (struct ethtool_cmd *) strings, + ETHTOOL_GSTRINGS, "ETHTOOL_GSTRINGS"); + if (error) { + goto out; + } + + for (int i = 0; i < len; i++) { + strings->data[(i + 1) * ETH_GSTRING_LEN - 1] = 0; + } + + *pstrings = strings; + + return 0; +out: + *pstrings = NULL; + free(strings); + return error; +} + +static void +netdev_linux_set_ol(struct netdev *netdev_) +{ + struct netdev_linux *netdev = netdev_linux_cast(netdev_); + struct ethtool_gstrings *names = NULL; + struct ethtool_gfeatures *features = NULL; + int error; + + COVERAGE_INC(netdev_get_ethtool); + + error = netdev_linux_read_definitions(netdev, &names); + if (error) { + return; + } + + features = xmalloc(sizeof *features + + DIV_ROUND_UP(names->len, 32) * + sizeof features->features[0]); + if (!features) { + goto out; + } + + features->cmd = ETHTOOL_GFEATURES; + features->size = DIV_ROUND_UP(names->len, 32); + error = netdev_linux_do_ethtool(netdev_get_name(netdev_), + (struct ethtool_cmd *) features, + ETHTOOL_GFEATURES, "ETHTOOL_GFEATURES"); + + if (error) { + goto out; + } + +#define FEATURE_WORD(blocks, index, field) ((blocks)[(index) / 32U].field) +#define FEATURE_FIELD_FLAG(index) (1U << (index) % 32U) +#define FEATURE_BIT_IS_SET(blocks, index, field) \ + (FEATURE_WORD(blocks, index, field) & FEATURE_FIELD_FLAG(index)) + + netdev->up.ol_flags = 0; + static const struct { + char * string; + int value; + } t_list[] = { + {"tx-checksum-ipv4", NETDEV_TX_OFFLOAD_IPV4_CKSUM | + NETDEV_TX_OFFLOAD_TCP_CKSUM | + NETDEV_TX_OFFLOAD_UDP_CKSUM}, + {"tx-checksum-ipv6", NETDEV_TX_OFFLOAD_TCP_CKSUM | + NETDEV_TX_OFFLOAD_UDP_CKSUM}, + {"tx-checksum-ip-generic", NETDEV_TX_OFFLOAD_IPV4_CKSUM | + NETDEV_TX_OFFLOAD_TCP_CKSUM | + NETDEV_TX_OFFLOAD_UDP_CKSUM}, + {"tx-checksum-sctp", NETDEV_TX_OFFLOAD_SCTP_CKSUM}, + {"tx-tcp-segmentation", NETDEV_TX_OFFLOAD_TCP_TSO}, + }; + + for (int i = 0; i < names->len; i++) { + char * name = (char *) names->data + i * ETH_GSTRING_LEN; + for (int j = 0; j < sizeof t_list / sizeof t_list[0]; j++) { + if (strcmp(t_list[j].string, name) == 0) { + if (FEATURE_BIT_IS_SET(features->features, i, active)) { + netdev_->ol_flags |= t_list[j].value; + } + } + } + } + +out: + free(names); + free(features); +} + static void netdev_linux_read_features(struct netdev_linux *netdev) { diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at index d2e6ac768..5a7b7a6e7 100644 --- a/tests/ofproto-macros.at +++ b/tests/ofproto-macros.at @@ -260,6 +260,7 @@ check_logs () { /ovs_rcu.*blocked [[0-9]]* ms waiting for .* to quiesce/d /Dropped [[0-9]]* log messages/d /setting extended ack support failed/d +/ETHTOOL_GSSET_INFO/d /|WARN|/p /|ERR|/p /|EMER|/p" ${logs}
Currently when userspace-tso is enabled, netdev-linux interfaces will indicate support for all offload flags regardless of interface configuration. This patch checks for which offload features are enabled during netdev construction. Signed-off-by: Mike Pattrick <mkp@redhat.com> --- lib/netdev-linux.c | 156 ++++++++++++++++++++++++++++++++++++++-- tests/ofproto-macros.at | 1 + 2 files changed, 150 insertions(+), 7 deletions(-)