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 <m...@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}
> --
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
