netdev - Re: [RFCv2 PATCH net-next 1/2] net-next: ethtool: extend ringparam set/get APIs for tx

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220326125042.216c9054@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Sat, 26 Mar 2022 12:50:42 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Jie Wang <wangjie125@...wei.com>
Cc:     <mkubecek@...e.cz>, <davem@...emloft.net>,
        <netdev@...r.kernel.org>, <huangguangbin2@...wei.com>,
        <lipeng321@...wei.com>, <shenjian15@...wei.com>,
        <moyufeng@...wei.com>, <linyunsheng@...wei.com>,
        <salil.mehta@...wei.com>, <chenhao288@...ilicon.com>
Subject: Re: [RFCv2 PATCH net-next 1/2] net-next: ethtool: extend ringparam
 set/get APIs for tx_push

On Sat, 26 Mar 2022 16:51:01 +0800 Jie Wang wrote:
> Currently tx push is a standard driver feature which controls use of a fast
> path descriptor push. So this patch extends the ringparam APIs and data
> structures to support set/get tx push by ethtool -G/g.
> 
> Signed-off-by: Jie Wang <wangjie125@...wei.com>
> ---
>  include/linux/ethtool.h              | 3 +++
>  include/uapi/linux/ethtool_netlink.h | 1 +
>  net/ethtool/netlink.h                | 2 +-
>  net/ethtool/rings.c                  | 9 +++++++--

You need to add documentation in:
Documentation/networking/ethtool-netlink.rst

> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index 4af58459a1e7..096771ee8586 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -72,11 +72,13 @@ enum {
>   * @rx_buf_len: Current length of buffers on the rx ring.
>   * @tcp_data_split: Scatter packet headers and data to separate buffers
>   * @cqe_size: Size of TX/RX completion queue event
> + * @tx_push: The flag of tx push mode
>   */
>  struct kernel_ethtool_ringparam {
>  	u32	rx_buf_len;
>  	u8	tcp_data_split;
>  	u32	cqe_size;
> +	u32	tx_push;

Can we make this a u8 and move it up above cqe_size?
u8 should be enough. You can use ethnl_update_u8().

>  };
>  
>  /**
> @@ -87,6 +89,7 @@ struct kernel_ethtool_ringparam {
>  enum ethtool_supported_ring_param {
>  	ETHTOOL_RING_USE_RX_BUF_LEN = BIT(0),
>  	ETHTOOL_RING_USE_CQE_SIZE   = BIT(1),
> +	ETHTOOL_RING_USE_TX_PUSH    = BIT(2),

You need to actually use this constant to reject the setting for
drivers which don't support the feature.

>  };
>  
>  #define __ETH_RSS_HASH_BIT(bit)	((u32)1 << (bit))

> @@ -94,7 +95,8 @@ static int rings_fill_reply(struct sk_buff *skb,
>  	     (nla_put_u8(skb, ETHTOOL_A_RINGS_TCP_DATA_SPLIT,
>  			 kr->tcp_data_split))) ||
>  	    (kr->cqe_size &&
> -	     (nla_put_u32(skb, ETHTOOL_A_RINGS_CQE_SIZE, kr->cqe_size))))
> +	     (nla_put_u32(skb, ETHTOOL_A_RINGS_CQE_SIZE, kr->cqe_size))) ||
> +	    nla_put_u8(skb, ETHTOOL_A_RINGS_TX_PUSH, !!kr->tx_push))
> 
>  		return -EMSGSIZE;
>  
>  	return 0;
> @@ -123,6 +125,7 @@ const struct nla_policy ethnl_rings_set_policy[] = {
>  	[ETHTOOL_A_RINGS_TX]			= { .type = NLA_U32 },
>  	[ETHTOOL_A_RINGS_RX_BUF_LEN]            = NLA_POLICY_MIN(NLA_U32, 1),
>  	[ETHTOOL_A_RINGS_CQE_SIZE]		= NLA_POLICY_MIN(NLA_U32, 1),
> +	[ETHTOOL_A_RINGS_TX_PUSH]		= { .type = NLA_U8 },

This can only be 0 and 1, right? Set a policy to to that effect please.

>  };
>  
>  int ethnl_set_rings(struct sk_buff *skb, struct genl_info *info)
> @@ -165,6 +168,8 @@ int ethnl_set_rings(struct sk_buff *skb, struct genl_info *info)
>  			 tb[ETHTOOL_A_RINGS_RX_BUF_LEN], &mod);
>  	ethnl_update_u32(&kernel_ringparam.cqe_size,
>  			 tb[ETHTOOL_A_RINGS_CQE_SIZE], &mod);
> +	ethnl_update_bool32(&kernel_ringparam.tx_push,
> +			    tb[ETHTOOL_A_RINGS_TX_PUSH], &mod);
>  	ret = 0;
>  	if (!mod)
>  		goto out_ops;