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: <3bc30bb8-f318-3e15-e61f-b430375b3739@huawei.com>
Date:   Mon, 28 Mar 2022 11:05:26 +0800
From:   "wangjie (L)" <wangjie125@...wei.com>
To:     Jakub Kicinski <kuba@...nel.org>
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 2022/3/27 3:50, Jakub Kicinski wrote:
> 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
>
OK, i will add it in next version RFC version.
>> 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().
>
yes, u8 is enough, next RFC version i will change it.
>>  };
>>
>>  /**
>> @@ -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.
>
I will use this constant next version.
>>  };
>>
>>  #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.
>
I will use NLA_POLICY_MAX(NLA_U8, 1), is this ok?
>>  };
>>
>>  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;
>
>
> .
>