| Message ID | 20220329091913.17869-2-wangjie125@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net-next: ethool: add support to get/set tx push by ethtool -G/g | expand |
On Tue, Mar 29, 2022 at 05:19:12PM +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@huawei.com> > --- [...] > diff --git a/net/ethtool/rings.c b/net/ethtool/rings.c > index 9f33c9689b56..2bc2d91f2e66 100644 > --- a/net/ethtool/rings.c > +++ b/net/ethtool/rings.c [...] > @@ -205,6 +210,15 @@ int ethnl_set_rings(struct sk_buff *skb, struct genl_info *info) > goto out_ops; > } > > + if (kernel_ringparam.tx_push && > + !(ops->supported_ring_params & ETHTOOL_RING_USE_TX_PUSH)) { > + ret = -EOPNOTSUPP; > + NL_SET_ERR_MSG_ATTR(info->extack, > + tb[ETHTOOL_A_RINGS_TX_PUSH], > + "setting tx push not supported"); > + goto out_ops; > + } > + > ret = dev->ethtool_ops->set_ringparam(dev, &ringparam, > &kernel_ringparam, info->extack); > if (ret < 0) This only disallows setting the parameter to true but allows requests trying to set it to false. I would rather prefer if (tb[ETHTOOL_A_RINGS_TX_PUSH] && !(ops->supported_ring_params & ETHTOOL_RING_USE_TX_PUSH)) { ... } and putting the check before rtnl_lock() so that we do not do useless work if the request is invalid. But the same can be said about the two checks we already have so those should be probably changed and moved as well. Michal
On Tue, 29 Mar 2022 17:19:12 +0800 Jie Wang wrote: > diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst > index 24d9be69065d..424159027309 100644 > --- a/Documentation/networking/ethtool-netlink.rst > +++ b/Documentation/networking/ethtool-netlink.rst > @@ -862,6 +862,7 @@ Kernel response contents: > ``ETHTOOL_A_RINGS_RX_BUF_LEN`` u32 size of buffers on the ring > ``ETHTOOL_A_RINGS_TCP_DATA_SPLIT`` u8 TCP header / data split > ``ETHTOOL_A_RINGS_CQE_SIZE`` u32 Size of TX/RX CQE > + ``ETHTOOL_A_RINGS_TX_PUSH`` u8 flag of TX Push mode > ==================================== ====== =========================== > > ``ETHTOOL_A_RINGS_TCP_DATA_SPLIT`` indicates whether the device is usable with > @@ -887,6 +888,7 @@ Request contents: > ``ETHTOOL_A_RINGS_TX`` u32 size of TX ring > ``ETHTOOL_A_RINGS_RX_BUF_LEN`` u32 size of buffers on the ring > ``ETHTOOL_A_RINGS_CQE_SIZE`` u32 Size of TX/RX CQE > + ``ETHTOOL_A_RINGS_TX_PUSH`` u8 flag of TX Push mode > ==================================== ====== =========================== > > Kernel checks that requested ring sizes do not exceed limits reported by You need to also describe what it does. Do you have a user manual or some form of feature documentation that could be used as a starting point. We're happy to help with the wording and grammar but you need to give us a description of the feature so we're not guessing.
On 2022/3/30 3:09, Michal Kubecek wrote: > On Tue, Mar 29, 2022 at 05:19:12PM +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@huawei.com> >> --- > [...] >> diff --git a/net/ethtool/rings.c b/net/ethtool/rings.c >> index 9f33c9689b56..2bc2d91f2e66 100644 >> --- a/net/ethtool/rings.c >> +++ b/net/ethtool/rings.c > [...] >> @@ -205,6 +210,15 @@ int ethnl_set_rings(struct sk_buff *skb, struct genl_info *info) >> goto out_ops; >> } >> >> + if (kernel_ringparam.tx_push && >> + !(ops->supported_ring_params & ETHTOOL_RING_USE_TX_PUSH)) { >> + ret = -EOPNOTSUPP; >> + NL_SET_ERR_MSG_ATTR(info->extack, >> + tb[ETHTOOL_A_RINGS_TX_PUSH], >> + "setting tx push not supported"); >> + goto out_ops; >> + } >> + >> ret = dev->ethtool_ops->set_ringparam(dev, &ringparam, >> &kernel_ringparam, info->extack); >> if (ret < 0) > > This only disallows setting the parameter to true but allows requests > trying to set it to false. I would rather prefer > > if (tb[ETHTOOL_A_RINGS_TX_PUSH] && > !(ops->supported_ring_params & ETHTOOL_RING_USE_TX_PUSH)) { > ... > } > > and putting the check before rtnl_lock() so that we do not do useless > work if the request is invalid. > > But the same can be said about the two checks we already have so those > should be probably changed and moved as well. > > Michal OK, I will move these three checks in a new patch. >
On 2022/3/30 7:20, Jakub Kicinski wrote: > On Tue, 29 Mar 2022 17:19:12 +0800 Jie Wang wrote: >> diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst >> index 24d9be69065d..424159027309 100644 >> --- a/Documentation/networking/ethtool-netlink.rst >> +++ b/Documentation/networking/ethtool-netlink.rst >> @@ -862,6 +862,7 @@ Kernel response contents: >> ``ETHTOOL_A_RINGS_RX_BUF_LEN`` u32 size of buffers on the ring >> ``ETHTOOL_A_RINGS_TCP_DATA_SPLIT`` u8 TCP header / data split >> ``ETHTOOL_A_RINGS_CQE_SIZE`` u32 Size of TX/RX CQE >> + ``ETHTOOL_A_RINGS_TX_PUSH`` u8 flag of TX Push mode >> ==================================== ====== =========================== >> >> ``ETHTOOL_A_RINGS_TCP_DATA_SPLIT`` indicates whether the device is usable with >> @@ -887,6 +888,7 @@ Request contents: >> ``ETHTOOL_A_RINGS_TX`` u32 size of TX ring >> ``ETHTOOL_A_RINGS_RX_BUF_LEN`` u32 size of buffers on the ring >> ``ETHTOOL_A_RINGS_CQE_SIZE`` u32 Size of TX/RX CQE >> + ``ETHTOOL_A_RINGS_TX_PUSH`` u8 flag of TX Push mode >> ==================================== ====== =========================== >> >> Kernel checks that requested ring sizes do not exceed limits reported by > > You need to also describe what it does. Do you have a user manual > or some form of feature documentation that could be used as a starting > point. We're happy to help with the wording and grammar but you need > to give us a description of the feature so we're not guessing. yes, I have some feature focumentations. I will add this part in next version. > > . >
diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst index 24d9be69065d..424159027309 100644 --- a/Documentation/networking/ethtool-netlink.rst +++ b/Documentation/networking/ethtool-netlink.rst @@ -862,6 +862,7 @@ Kernel response contents: ``ETHTOOL_A_RINGS_RX_BUF_LEN`` u32 size of buffers on the ring ``ETHTOOL_A_RINGS_TCP_DATA_SPLIT`` u8 TCP header / data split ``ETHTOOL_A_RINGS_CQE_SIZE`` u32 Size of TX/RX CQE + ``ETHTOOL_A_RINGS_TX_PUSH`` u8 flag of TX Push mode ==================================== ====== =========================== ``ETHTOOL_A_RINGS_TCP_DATA_SPLIT`` indicates whether the device is usable with @@ -887,6 +888,7 @@ Request contents: ``ETHTOOL_A_RINGS_TX`` u32 size of TX ring ``ETHTOOL_A_RINGS_RX_BUF_LEN`` u32 size of buffers on the ring ``ETHTOOL_A_RINGS_CQE_SIZE`` u32 Size of TX/RX CQE + ``ETHTOOL_A_RINGS_TX_PUSH`` u8 flag of TX Push mode ==================================== ====== =========================== Kernel checks that requested ring sizes do not exceed limits reported by diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h index 4af58459a1e7..ede4f9154cd2 100644 --- a/include/linux/ethtool.h +++ b/include/linux/ethtool.h @@ -71,11 +71,13 @@ enum { * struct kernel_ethtool_ringparam - RX/TX ring configuration * @rx_buf_len: Current length of buffers on the rx ring. * @tcp_data_split: Scatter packet headers and data to separate buffers + * @tx_push: The flag of tx push mode * @cqe_size: Size of TX/RX completion queue event */ struct kernel_ethtool_ringparam { u32 rx_buf_len; u8 tcp_data_split; + u8 tx_push; u32 cqe_size; }; @@ -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), }; #define __ETH_RSS_HASH_BIT(bit) ((u32)1 << (bit)) diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h index 979850221b8d..d2fb4f7be61b 100644 --- a/include/uapi/linux/ethtool_netlink.h +++ b/include/uapi/linux/ethtool_netlink.h @@ -338,6 +338,7 @@ enum { ETHTOOL_A_RINGS_RX_BUF_LEN, /* u32 */ ETHTOOL_A_RINGS_TCP_DATA_SPLIT, /* u8 */ ETHTOOL_A_RINGS_CQE_SIZE, /* u32 */ + ETHTOOL_A_RINGS_TX_PUSH, /* u8 */ /* add new constants above here */ __ETHTOOL_A_RINGS_CNT, diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h index 29d01662a48b..7919ddb2371c 100644 --- a/net/ethtool/netlink.h +++ b/net/ethtool/netlink.h @@ -363,7 +363,7 @@ extern const struct nla_policy ethnl_features_set_policy[ETHTOOL_A_FEATURES_WANT extern const struct nla_policy ethnl_privflags_get_policy[ETHTOOL_A_PRIVFLAGS_HEADER + 1]; extern const struct nla_policy ethnl_privflags_set_policy[ETHTOOL_A_PRIVFLAGS_FLAGS + 1]; extern const struct nla_policy ethnl_rings_get_policy[ETHTOOL_A_RINGS_HEADER + 1]; -extern const struct nla_policy ethnl_rings_set_policy[ETHTOOL_A_RINGS_CQE_SIZE + 1]; +extern const struct nla_policy ethnl_rings_set_policy[ETHTOOL_A_RINGS_TX_PUSH + 1]; extern const struct nla_policy ethnl_channels_get_policy[ETHTOOL_A_CHANNELS_HEADER + 1]; extern const struct nla_policy ethnl_channels_set_policy[ETHTOOL_A_CHANNELS_COMBINED_COUNT + 1]; extern const struct nla_policy ethnl_coalesce_get_policy[ETHTOOL_A_COALESCE_HEADER + 1]; diff --git a/net/ethtool/rings.c b/net/ethtool/rings.c index 9f33c9689b56..2bc2d91f2e66 100644 --- a/net/ethtool/rings.c +++ b/net/ethtool/rings.c @@ -55,7 +55,8 @@ static int rings_reply_size(const struct ethnl_req_info *req_base, nla_total_size(sizeof(u32)) + /* _RINGS_TX */ nla_total_size(sizeof(u32)) + /* _RINGS_RX_BUF_LEN */ nla_total_size(sizeof(u8)) + /* _RINGS_TCP_DATA_SPLIT */ - nla_total_size(sizeof(u32)); /* _RINGS_CQE_SIZE */ + nla_total_size(sizeof(u32) + /* _RINGS_CQE_SIZE */ + nla_total_size(sizeof(u8))); /* _RINGS_TX_PUSH */ } static int rings_fill_reply(struct sk_buff *skb, @@ -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] = NLA_POLICY_MAX(NLA_U8, 1), }; 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_u8(&kernel_ringparam.tx_push, + tb[ETHTOOL_A_RINGS_TX_PUSH], &mod); ret = 0; if (!mod) goto out_ops; @@ -205,6 +210,15 @@ int ethnl_set_rings(struct sk_buff *skb, struct genl_info *info) goto out_ops; } + if (kernel_ringparam.tx_push && + !(ops->supported_ring_params & ETHTOOL_RING_USE_TX_PUSH)) { + ret = -EOPNOTSUPP; + NL_SET_ERR_MSG_ATTR(info->extack, + tb[ETHTOOL_A_RINGS_TX_PUSH], + "setting tx push not supported"); + goto out_ops; + } + ret = dev->ethtool_ops->set_ringparam(dev, &ringparam, &kernel_ringparam, info->extack); if (ret < 0)
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@huawei.com> --- Documentation/networking/ethtool-netlink.rst | 2 ++ include/linux/ethtool.h | 3 +++ include/uapi/linux/ethtool_netlink.h | 1 + net/ethtool/netlink.h | 2 +- net/ethtool/rings.c | 18 ++++++++++++++++-- 5 files changed, 23 insertions(+), 3 deletions(-)