| Message ID | 20241003160620.1521626-6-ap420073@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | bnxt_en: implement device memory TCP for bnxt | expand |
On Thu, Oct 3, 2024 at 9:07 AM Taehee Yoo <ap420073@gmail.com> wrote: > > If driver doesn't support ring parameter or tcp-data-split configuration > is not sufficient, the devmem should not be set up. > Before setup the devmem, tcp-data-split should be ON and > tcp-data-split-thresh value should be 0. > > Signed-off-by: Taehee Yoo <ap420073@gmail.com> > --- > > v3: > - Patch added. > > net/core/devmem.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/net/core/devmem.c b/net/core/devmem.c > index 11b91c12ee11..a9e9b15028e0 100644 > --- a/net/core/devmem.c > +++ b/net/core/devmem.c > @@ -8,6 +8,8 @@ > */ > > #include <linux/dma-buf.h> > +#include <linux/ethtool.h> > +#include <linux/ethtool_netlink.h> > #include <linux/genalloc.h> > #include <linux/mm.h> > #include <linux/netdevice.h> > @@ -131,6 +133,8 @@ int net_devmem_bind_dmabuf_to_queue(struct net_device *dev, u32 rxq_idx, > struct net_devmem_dmabuf_binding *binding, > struct netlink_ext_ack *extack) > { > + struct kernel_ethtool_ringparam kernel_ringparam = {}; > + struct ethtool_ringparam ringparam = {}; > struct netdev_rx_queue *rxq; > u32 xa_idx; > int err; > @@ -146,6 +150,20 @@ int net_devmem_bind_dmabuf_to_queue(struct net_device *dev, u32 rxq_idx, > return -EEXIST; > } > > + if (!dev->ethtool_ops->get_ringparam) { > + NL_SET_ERR_MSG(extack, "can't get ringparam"); > + return -EINVAL; > + } > + > + dev->ethtool_ops->get_ringparam(dev, &ringparam, > + &kernel_ringparam, extack); > + if (kernel_ringparam.tcp_data_split != ETHTOOL_TCP_DATA_SPLIT_ENABLED || The way I had set this up is that the driver checks whether header split is enabled, and only sets PP_FLAG_ALLOW_UNREADABLE_NETMEM if it is. Then core detects that the driver did not allow unreadable netmem and it fails that way. This check is redundant with that. I'm not 100% opposed to redundant checks. Maybe they will add some reliability, but also maybe they will be confusing to check the same thing essentially in 2 places. Is the PP_FLAG_ALLOW_UNREADABLE_NETMEM trick not sufficient for you? > + kernel_ringparam.tcp_data_split_thresh) { > + NL_SET_ERR_MSG(extack, > + "tcp-header-data-split is disabled or threshold is not zero"); > + return -EINVAL; > + } > + > #ifdef CONFIG_XDP_SOCKETS > if (rxq->pool) { > NL_SET_ERR_MSG(extack, "designated queue already in use by AF_XDP"); > -- > 2.34.1 >
On 10/3/2024 9:06 AM, Taehee Yoo wrote: > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding. > > > If driver doesn't support ring parameter or tcp-data-split configuration > is not sufficient, the devmem should not be set up. > Before setup the devmem, tcp-data-split should be ON and > tcp-data-split-thresh value should be 0. > > Signed-off-by: Taehee Yoo <ap420073@gmail.com> > --- > > v3: > - Patch added. > > net/core/devmem.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/net/core/devmem.c b/net/core/devmem.c > index 11b91c12ee11..a9e9b15028e0 100644 > --- a/net/core/devmem.c > +++ b/net/core/devmem.c > @@ -8,6 +8,8 @@ > */ > > #include <linux/dma-buf.h> > +#include <linux/ethtool.h> > +#include <linux/ethtool_netlink.h> > #include <linux/genalloc.h> > #include <linux/mm.h> > #include <linux/netdevice.h> > @@ -131,6 +133,8 @@ int net_devmem_bind_dmabuf_to_queue(struct net_device *dev, u32 rxq_idx, > struct net_devmem_dmabuf_binding *binding, > struct netlink_ext_ack *extack) > { > + struct kernel_ethtool_ringparam kernel_ringparam = {}; > + struct ethtool_ringparam ringparam = {}; > struct netdev_rx_queue *rxq; > u32 xa_idx; > int err; > @@ -146,6 +150,20 @@ int net_devmem_bind_dmabuf_to_queue(struct net_device *dev, u32 rxq_idx, > return -EEXIST; > } > > + if (!dev->ethtool_ops->get_ringparam) { > + NL_SET_ERR_MSG(extack, "can't get ringparam"); > + return -EINVAL; > + } Is EINVAL the correct return value here? I think it makes more sense as EOPNOTSUPP. > + > + dev->ethtool_ops->get_ringparam(dev, &ringparam, > + &kernel_ringparam, extack); > + if (kernel_ringparam.tcp_data_split != ETHTOOL_TCP_DATA_SPLIT_ENABLED || > + kernel_ringparam.tcp_data_split_thresh) { > + NL_SET_ERR_MSG(extack, > + "tcp-header-data-split is disabled or threshold is not zero"); > + return -EINVAL; > + } > + Maybe just my personal opinion, but IMHO these checks should be separate so the error message can be more concise/clear. Also, a small nit, but I think both of these checks should be before getting the rxq via __netif_get_rx_queue(). Thanks, Brett > #ifdef CONFIG_XDP_SOCKETS > if (rxq->pool) { > NL_SET_ERR_MSG(extack, "designated queue already in use by AF_XDP"); > -- > 2.34.1 >
On Thu, Oct 3, 2024 at 11:35 AM Brett Creeley <bcreeley@amd.com> wrote: > > > > On 10/3/2024 9:06 AM, Taehee Yoo wrote: > > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding. > > > > > > If driver doesn't support ring parameter or tcp-data-split configuration > > is not sufficient, the devmem should not be set up. > > Before setup the devmem, tcp-data-split should be ON and > > tcp-data-split-thresh value should be 0. > > > > Signed-off-by: Taehee Yoo <ap420073@gmail.com> > > --- > > > > v3: > > - Patch added. > > > > net/core/devmem.c | 18 ++++++++++++++++++ > > 1 file changed, 18 insertions(+) > > > > diff --git a/net/core/devmem.c b/net/core/devmem.c > > index 11b91c12ee11..a9e9b15028e0 100644 > > --- a/net/core/devmem.c > > +++ b/net/core/devmem.c > > @@ -8,6 +8,8 @@ > > */ > > > > #include <linux/dma-buf.h> > > +#include <linux/ethtool.h> > > +#include <linux/ethtool_netlink.h> > > #include <linux/genalloc.h> > > #include <linux/mm.h> > > #include <linux/netdevice.h> > > @@ -131,6 +133,8 @@ int net_devmem_bind_dmabuf_to_queue(struct net_device *dev, u32 rxq_idx, > > struct net_devmem_dmabuf_binding *binding, > > struct netlink_ext_ack *extack) > > { > > + struct kernel_ethtool_ringparam kernel_ringparam = {}; > > + struct ethtool_ringparam ringparam = {}; > > struct netdev_rx_queue *rxq; > > u32 xa_idx; > > int err; > > @@ -146,6 +150,20 @@ int net_devmem_bind_dmabuf_to_queue(struct net_device *dev, u32 rxq_idx, > > return -EEXIST; > > } > > > > + if (!dev->ethtool_ops->get_ringparam) { > > + NL_SET_ERR_MSG(extack, "can't get ringparam"); > > + return -EINVAL; > > + } > > Is EINVAL the correct return value here? I think it makes more sense as > EOPNOTSUPP. > > > + > > + dev->ethtool_ops->get_ringparam(dev, &ringparam, > > + &kernel_ringparam, extack); > > + if (kernel_ringparam.tcp_data_split != ETHTOOL_TCP_DATA_SPLIT_ENABLED || > > + kernel_ringparam.tcp_data_split_thresh) { > > + NL_SET_ERR_MSG(extack, > > + "tcp-header-data-split is disabled or threshold is not zero"); > > + return -EINVAL; > > + } > > + > Maybe just my personal opinion, but IMHO these checks should be separate > so the error message can be more concise/clear. > Good point. The error message in itself is valuable.
On Fri, Oct 4, 2024 at 3:29 AM Mina Almasry <almasrymina@google.com> wrote: > Hi Mina, Thanks a lot for the review! > On Thu, Oct 3, 2024 at 9:07 AM Taehee Yoo <ap420073@gmail.com> wrote: > > > > If driver doesn't support ring parameter or tcp-data-split configuration > > is not sufficient, the devmem should not be set up. > > Before setup the devmem, tcp-data-split should be ON and > > tcp-data-split-thresh value should be 0. > > > > Signed-off-by: Taehee Yoo <ap420073@gmail.com> > > --- > > > > v3: > > - Patch added. > > > > net/core/devmem.c | 18 ++++++++++++++++++ > > 1 file changed, 18 insertions(+) > > > > diff --git a/net/core/devmem.c b/net/core/devmem.c > > index 11b91c12ee11..a9e9b15028e0 100644 > > --- a/net/core/devmem.c > > +++ b/net/core/devmem.c > > @@ -8,6 +8,8 @@ > > */ > > > > #include <linux/dma-buf.h> > > +#include <linux/ethtool.h> > > +#include <linux/ethtool_netlink.h> > > #include <linux/genalloc.h> > > #include <linux/mm.h> > > #include <linux/netdevice.h> > > @@ -131,6 +133,8 @@ int net_devmem_bind_dmabuf_to_queue(struct net_device *dev, u32 rxq_idx, > > struct net_devmem_dmabuf_binding *binding, > > struct netlink_ext_ack *extack) > > { > > + struct kernel_ethtool_ringparam kernel_ringparam = {}; > > + struct ethtool_ringparam ringparam = {}; > > struct netdev_rx_queue *rxq; > > u32 xa_idx; > > int err; > > @@ -146,6 +150,20 @@ int net_devmem_bind_dmabuf_to_queue(struct net_device *dev, u32 rxq_idx, > > return -EEXIST; > > } > > > > + if (!dev->ethtool_ops->get_ringparam) { > > + NL_SET_ERR_MSG(extack, "can't get ringparam"); > > + return -EINVAL; > > + } > > + > > + dev->ethtool_ops->get_ringparam(dev, &ringparam, > > + &kernel_ringparam, extack); > > + if (kernel_ringparam.tcp_data_split != ETHTOOL_TCP_DATA_SPLIT_ENABLED || > > The way I had set this up is that the driver checks whether header > split is enabled, and only sets PP_FLAG_ALLOW_UNREADABLE_NETMEM if it > is. Then core detects that the driver did not allow unreadable netmem > and it fails that way. > > This check is redundant with that. I'm not 100% opposed to redundant > checks. Maybe they will add some reliability, but also maybe they will > be confusing to check the same thing essentially in 2 places. > > Is the PP_FLAG_ALLOW_UNREADABLE_NETMEM trick not sufficient for you? Ah okay, I understand. It looks like it's already validated enough based on PP_FLAG_ALLOW_UNREADABLE_NETMEM. I tested how you guided it, and it works as you intended. It's a duplicated validation indeed, so I will drop this patch in a v4. Thanks a lot! Taehee Yoo > > > + kernel_ringparam.tcp_data_split_thresh) { > > + NL_SET_ERR_MSG(extack, > > + "tcp-header-data-split is disabled or threshold is not zero"); > > + return -EINVAL; > > + } > > + > > #ifdef CONFIG_XDP_SOCKETS > > if (rxq->pool) { > > NL_SET_ERR_MSG(extack, "designated queue already in use by AF_XDP"); > > -- > > 2.34.1 > > > > > -- > Thanks, > Mina
On Fri, Oct 4, 2024 at 3:35 AM Brett Creeley <bcreeley@amd.com> wrote: > Hi Brett, Thanks a lot for your review! > > > On 10/3/2024 9:06 AM, Taehee Yoo wrote: > > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding. > > > > > > If driver doesn't support ring parameter or tcp-data-split configuration > > is not sufficient, the devmem should not be set up. > > Before setup the devmem, tcp-data-split should be ON and > > tcp-data-split-thresh value should be 0. > > > > Signed-off-by: Taehee Yoo <ap420073@gmail.com> > > --- > > > > v3: > > - Patch added. > > > > net/core/devmem.c | 18 ++++++++++++++++++ > > 1 file changed, 18 insertions(+) > > > > diff --git a/net/core/devmem.c b/net/core/devmem.c > > index 11b91c12ee11..a9e9b15028e0 100644 > > --- a/net/core/devmem.c > > +++ b/net/core/devmem.c > > @@ -8,6 +8,8 @@ > > */ > > > > #include <linux/dma-buf.h> > > +#include <linux/ethtool.h> > > +#include <linux/ethtool_netlink.h> > > #include <linux/genalloc.h> > > #include <linux/mm.h> > > #include <linux/netdevice.h> > > @@ -131,6 +133,8 @@ int net_devmem_bind_dmabuf_to_queue(struct net_device *dev, u32 rxq_idx, > > struct net_devmem_dmabuf_binding *binding, > > struct netlink_ext_ack *extack) > > { > > + struct kernel_ethtool_ringparam kernel_ringparam = {}; > > + struct ethtool_ringparam ringparam = {}; > > struct netdev_rx_queue *rxq; > > u32 xa_idx; > > int err; > > @@ -146,6 +150,20 @@ int net_devmem_bind_dmabuf_to_queue(struct net_device *dev, u32 rxq_idx, > > return -EEXIST; > > } > > > > + if (!dev->ethtool_ops->get_ringparam) { > > + NL_SET_ERR_MSG(extack, "can't get ringparam"); > > + return -EINVAL; > > + } > > Is EINVAL the correct return value here? I think it makes more sense as > EOPNOTSUPP. Yes, Thanks for catching this. > > > + > > + dev->ethtool_ops->get_ringparam(dev, &ringparam, > > + &kernel_ringparam, extack); > > + if (kernel_ringparam.tcp_data_split != ETHTOOL_TCP_DATA_SPLIT_ENABLED || > > + kernel_ringparam.tcp_data_split_thresh) { > > + NL_SET_ERR_MSG(extack, > > + "tcp-header-data-split is disabled or threshold is not zero"); > > + return -EINVAL; > > + } > > + > Maybe just my personal opinion, but IMHO these checks should be separate > so the error message can be more concise/clear. I agree, the error message is not clear, it contains two conditions. > > Also, a small nit, but I think both of these checks should be before > getting the rxq via __netif_get_rx_queue(). > I will drop this patch in a v4 patch. Thanks a lot! Taehee Yoo > > Thanks, > > Brett > > #ifdef CONFIG_XDP_SOCKETS > > if (rxq->pool) { > > NL_SET_ERR_MSG(extack, "designated queue already in use by AF_XDP"); > > -- > > 2.34.1 > >
On Thu, 3 Oct 2024 11:49:50 -0700 Mina Almasry wrote: > > > + dev->ethtool_ops->get_ringparam(dev, &ringparam, > > > + &kernel_ringparam, extack); > > > + if (kernel_ringparam.tcp_data_split != ETHTOOL_TCP_DATA_SPLIT_ENABLED || > > > + kernel_ringparam.tcp_data_split_thresh) { > > > + NL_SET_ERR_MSG(extack, > > > + "tcp-header-data-split is disabled or threshold is not zero"); > > > + return -EINVAL; > > > + } > > > + > > Maybe just my personal opinion, but IMHO these checks should be separate > > so the error message can be more concise/clear. > > > > Good point. The error message in itself is valuable. If you mean that the error message is more intuitive than debugging why PP_FLAG_ALLOW_UNREADABLE_NETMEM isn't set - I agree :) I vote to keep the patch, FWIW. Maybe add a comment that for now drivers should not set PP_FLAG_ALLOW_UNREADABLE_NETMEM, anyway, but this gives us better debuggability, and in the future we may find cases where doing a copy is cheaper than buffer circulation (and therefore may lift this check).
On Wed, Oct 9, 2024 at 4:28 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Thu, 3 Oct 2024 11:49:50 -0700 Mina Almasry wrote: > > > > + dev->ethtool_ops->get_ringparam(dev, &ringparam, > > > > + &kernel_ringparam, extack); > > > > + if (kernel_ringparam.tcp_data_split != ETHTOOL_TCP_DATA_SPLIT_ENABLED || > > > > + kernel_ringparam.tcp_data_split_thresh) { > > > > + NL_SET_ERR_MSG(extack, > > > > + "tcp-header-data-split is disabled or threshold is not zero"); > > > > + return -EINVAL; > > > > + } > > > > + > > > Maybe just my personal opinion, but IMHO these checks should be separate > > > so the error message can be more concise/clear. > > > > > > > Good point. The error message in itself is valuable. > > If you mean that the error message is more intuitive than debugging why > PP_FLAG_ALLOW_UNREADABLE_NETMEM isn't set - I agree :) > > I vote to keep the patch, FWIW. Maybe add a comment that for now drivers > should not set PP_FLAG_ALLOW_UNREADABLE_NETMEM, anyway, but this gives > us better debuggability, and in the future we may find cases where > doing a copy is cheaper than buffer circulation (and therefore may lift > this check). Okay, I will not drop this patch in v4 patch. So, I just will fix what Brett and Mina pointed out.
diff --git a/net/core/devmem.c b/net/core/devmem.c index 11b91c12ee11..a9e9b15028e0 100644 --- a/net/core/devmem.c +++ b/net/core/devmem.c @@ -8,6 +8,8 @@ */ #include <linux/dma-buf.h> +#include <linux/ethtool.h> +#include <linux/ethtool_netlink.h> #include <linux/genalloc.h> #include <linux/mm.h> #include <linux/netdevice.h> @@ -131,6 +133,8 @@ int net_devmem_bind_dmabuf_to_queue(struct net_device *dev, u32 rxq_idx, struct net_devmem_dmabuf_binding *binding, struct netlink_ext_ack *extack) { + struct kernel_ethtool_ringparam kernel_ringparam = {}; + struct ethtool_ringparam ringparam = {}; struct netdev_rx_queue *rxq; u32 xa_idx; int err; @@ -146,6 +150,20 @@ int net_devmem_bind_dmabuf_to_queue(struct net_device *dev, u32 rxq_idx, return -EEXIST; } + if (!dev->ethtool_ops->get_ringparam) { + NL_SET_ERR_MSG(extack, "can't get ringparam"); + return -EINVAL; + } + + dev->ethtool_ops->get_ringparam(dev, &ringparam, + &kernel_ringparam, extack); + if (kernel_ringparam.tcp_data_split != ETHTOOL_TCP_DATA_SPLIT_ENABLED || + kernel_ringparam.tcp_data_split_thresh) { + NL_SET_ERR_MSG(extack, + "tcp-header-data-split is disabled or threshold is not zero"); + return -EINVAL; + } + #ifdef CONFIG_XDP_SOCKETS if (rxq->pool) { NL_SET_ERR_MSG(extack, "designated queue already in use by AF_XDP");
If driver doesn't support ring parameter or tcp-data-split configuration is not sufficient, the devmem should not be set up. Before setup the devmem, tcp-data-split should be ON and tcp-data-split-thresh value should be 0. Signed-off-by: Taehee Yoo <ap420073@gmail.com> --- v3: - Patch added. net/core/devmem.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)