[ovs-dev,v6,1/3] netdev-linux: Use ethtool to detect offload support.

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>

--

v6:
 - Removed legacy comment
 - Reverse xmas
---
 lib/netdev-linux.c      | 146 ++++++++++++++++++++++++++++++++++++++--
 tests/ofproto-macros.at |   1 +
 2 files changed, 140 insertions(+), 7 deletions(-)

--
2.39.3

On Tue, Oct 31, 2023 at 03:51:36PM -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>
> 
> --
> 
> v6:
>  - Removed legacy comment
>  - Reverse xmas

Thanks for the updates.
This patch looks good to me.

Acked-by: Simon Horman <horms@ovn.org>

For the record, this entire series seems ready to me.
But I think it is appropriate for Ilya to have the option to review the
series.

On 10/31/23 20:51, 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>
> 
> --
> 
> v6:
>  - Removed legacy comment
>  - Reverse xmas
> ---
>  lib/netdev-linux.c      | 146 ++++++++++++++++++++++++++++++++++++++--
>  tests/ofproto-macros.at |   1 +
>  2 files changed, 140 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index cca340879..a46f5127f 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. */

This comment seems to be still sort of valid.  i.e. we're only calling the
netdev_linux_set_ol() when TSO is enabled, because if we call it just for
the checksum, we'll get TSO flags as well.  Maybe just re-phrase it instead
of removing?  Or maybe even keep as is?

>      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,143 @@ netdev_internal_get_stats(const struct netdev *netdev_,
>      return error;
>  }
> 
> +static int
> +netdev_linux_read_stringset_info(struct netdev_linux *netdev, uint32_t *len)
> +{
> +    union {
> +        struct ethtool_sset_info hdr;
> +        struct {
> +            uint64_t pad[2];
> +            uint32_t sset_len[1];
> +        };
> +    } sset_info;
> +    int error;
> +
> +    sset_info.hdr.cmd = ETHTOOL_GSSET_INFO;
> +    sset_info.hdr.reserved = 0;
> +    sset_info.hdr.sset_mask = 1ULL << ETH_SS_FEATURES;
> +
> +    error = netdev_linux_do_ethtool(netdev->up.name,

netdev_get_name(&netdev->up)

> +            (struct ethtool_cmd *)&sset_info,

'''
Put a space between the ``()`` used in a cast and the expression
whose type is cast: ``(void *) 0``.
'''

> +            ETHTOOL_GSSET_INFO, "ETHTOOL_GSSET_INFO");

Please, indent to the level of '('.

> +    if (error) {
> +        return error;
> +    }
> +    *len = sset_info.sset_len[0];

According to the ethtool API, the sset_mask is also an output
parameter.  We should probably check that our ETH_SS_FEATURES
flag is set there.  If not, the access above may be invalid.

> +    return 0;
> +}
> +
> +
> +static int
> +netdev_linux_read_definitions(struct netdev_linux *netdev,
> +                              struct ethtool_gstrings **pstrings)
> +{
> +    struct ethtool_gstrings *strings = NULL;
> +    uint32_t len = 0;
> +    int error = 0;
> +
> +    error = netdev_linux_read_stringset_info(netdev, &len);
> +    if (error || !len) {
> +        return error;
> +    }
> +    strings = xcalloc(1, sizeof(*strings) + len * ETH_GSTRING_LEN);

This should be just xzalloc.
And don't parenthesize the argument of sizeof.

> +    if (!strings) {
> +        return ENOMEM;

xcalloc can't fail, hence the 'x'.

> +    }
> +
> +    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");

Indentation.

> +    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_gfeatures *features = NULL;
> +    struct ethtool_gstrings *names = 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;

xmalloc can't fail.  Should it be xzalloc just to be sure?

> +    }
> +
> +    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");

Indentation.

> +
> +    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;

Extra space between '*' and the 'string'.

> +        int value;

Should value be uint32_t instead?  ol_flags is uint32_t.

> +    } 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;

Extra space  ----^

> +        for (int j = 0; j < sizeof t_list / sizeof t_list[0]; j++) {

ARRAY_SIZE(t_list)

> +            if (strcmp(t_list[j].string, name) == 0) {
> +                if (FEATURE_BIT_IS_SET(features->features, i, active)) {
> +                    netdev_->ol_flags |= t_list[j].value;

Above if statements could be combined with &&.
Also, shouldn't we break here?

If we break, we may also consider changing the order of nested loops,
i.e. iterate over t_list and look for a value in the names->data.
There is no need to serach for values we're not insterested in.

> +                }
> +            }
> +        }
> +    }
> +
> +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}
> --