| Message ID | 20241022162359.2713094-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 |
Hi Taehee, sorry for the late reply. I was out on vacation and needed to catch up on some stuff when I got back. On Tue, Oct 22, 2024 at 9:25 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 > header-data-split-thresh value should be 0. > > Tested-by: Stanislav Fomichev <sdf@fomichev.me> > Signed-off-by: Taehee Yoo <ap420073@gmail.com> > --- > > v4: > - Check condition before __netif_get_rx_queue(). > - Separate condition check. > - Add Test tag from Stanislav. > > 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..3425e872e87a 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; > @@ -140,6 +144,20 @@ int net_devmem_bind_dmabuf_to_queue(struct net_device *dev, u32 rxq_idx, > return -ERANGE; > } > > + if (!dev->ethtool_ops->get_ringparam) > + return -EOPNOTSUPP; > + Maybe an error code not EOPNOTSUPP. I think that gets returned when NET_DEVMEM is not compiled in and other situations like that. Lets pick another error code? Maybe ENODEV. Also consider extack error message. But it's very unlikely to hit this error, so maybe not necessary. > + dev->ethtool_ops->get_ringparam(dev, &ringparam, &kernel_ringparam, > + extack); > + if (kernel_ringparam.tcp_data_split != ETHTOOL_TCP_DATA_SPLIT_ENABLED) { > + NL_SET_ERR_MSG(extack, "tcp-data-split is disabled"); > + return -EINVAL; > + } > + if (kernel_ringparam.hds_thresh) { > + NL_SET_ERR_MSG(extack, "header-data-split-thresh is not zero"); > + return -EINVAL; > + } > + Thinking about drivers that support tcp-data-split, but don't support any kind of hds_thresh. For us (GVE), the hds_thresh is implicitly always 0. Does the driver need to explicitly set hds_thresh to 0? If so, that adds a bit of churn to driver code. Is it possible to detect in this function that the driver doesn't support hds_thresh and allow the binding if so? I see in the previous patch you do something like: supported_ring_params & ETHTOOL_RING_USE_HDS_THRS To detect there is hds_thresh support. I was wondering if something like this is possible so we don't have to update GVE and all future drivers to explicitly set thresh to 0. Other than that, looks fine to me.
On Fri, Nov 1, 2024 at 11:29 PM Mina Almasry <almasrymina@google.com> wrote: > > Hi Taehee, sorry for the late reply. I was out on vacation and needed > to catch up on some stuff when I got back. Hi Mina, Thank you so much for your review :) > > On Tue, Oct 22, 2024 at 9:25 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 > > header-data-split-thresh value should be 0. > > > > Tested-by: Stanislav Fomichev <sdf@fomichev.me> > > Signed-off-by: Taehee Yoo <ap420073@gmail.com> > > --- > > > > v4: > > - Check condition before __netif_get_rx_queue(). > > - Separate condition check. > > - Add Test tag from Stanislav. > > > > 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..3425e872e87a 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; > > @@ -140,6 +144,20 @@ int net_devmem_bind_dmabuf_to_queue(struct net_device *dev, u32 rxq_idx, > > return -ERANGE; > > } > > > > + if (!dev->ethtool_ops->get_ringparam) > > + return -EOPNOTSUPP; > > + > > Maybe an error code not EOPNOTSUPP. I think that gets returned when > NET_DEVMEM is not compiled in and other situations like that. Lets > pick another error code? Maybe ENODEV. There are several same code in the ethtool core. It returns EOPNOTSUPP consistently. In the v3 series, Brett reviewed it. So, I changed it from EINVAL to EOPNOTSUPP it was reasonable to me. So I prefer EOPNOTSUPP but I will follow your decision. What do you think? > > Also consider extack error message. But it's very unlikely to hit this > error, so maybe not necessary. I removed extack from the v3. because ethtool doesn't use extack for the same logic. It was reasonable to me. > > > + dev->ethtool_ops->get_ringparam(dev, &ringparam, &kernel_ringparam, > > + extack); > > + if (kernel_ringparam.tcp_data_split != ETHTOOL_TCP_DATA_SPLIT_ENABLED) { > > + NL_SET_ERR_MSG(extack, "tcp-data-split is disabled"); > > + return -EINVAL; > > + } > > + if (kernel_ringparam.hds_thresh) { > > + NL_SET_ERR_MSG(extack, "header-data-split-thresh is not zero"); > > + return -EINVAL; > > + } > > + > > Thinking about drivers that support tcp-data-split, but don't support > any kind of hds_thresh. For us (GVE), the hds_thresh is implicitly > always 0. > > Does the driver need to explicitly set hds_thresh to 0? If so, that > adds a bit of churn to driver code. Is it possible to detect in this > function that the driver doesn't support hds_thresh and allow the > binding if so? > > I see in the previous patch you do something like: > > supported_ring_params & ETHTOOL_RING_USE_HDS_THRS > > To detect there is hds_thresh support. I was wondering if something > like this is possible so we don't have to update GVE and all future > drivers to explicitly set thresh to 0. How about setting maximum hds_threshold to 0? The default value of hds_threshold of course 0. I think gve code does not need to change much, just adding like below will be okay. I think if drivers don't support setting hds_threshold explicitly, it is actually the same as support only 0. So, there is no problem I think. I didn't analyze gve driver code, So I might think it too optimistically. #define GVE_HDS_THRESHOLD_MAX 0 kernel_ering->hds_thresh = GVE_HDS_THRESHOLD_MAX; kernel_ering->hds_thresh_max = GVE_HDS_THRESHOLD_MAX; ... .supported_ring_params = ETHTOOL_RING_USE_TCP_DATA_SPLIT | ETHTOOL_RING_USE_HDS_THRS, ethtool command may show like this. ethtool -g enp7s0f3np3 Ring parameters for enp7s0f3np3: Pre-set maximums: ... Header data split thresh: 0 Current hardware settings: ... TCP data split: on Header data split thresh: 0 If a driver can't set up hds_threshold, ethtool command may show like this. ethtool -g enp7s0f3np3 Ring parameters for enp7s0f3np3: Pre-set maximums: TX push buff len: n/a Header data split thresh: n/a Current hardware settings: ... TCP data split: on Header data split thresh: n/a Thanks a lot! Taehee Yoo > > Other than that, looks fine to me. > > > -- > Thanks, > Mina
On Fri, Nov 1, 2024 at 11:03 AM Taehee Yoo <ap420073@gmail.com> wrote: > > On Fri, Nov 1, 2024 at 11:29 PM Mina Almasry <almasrymina@google.com> wrote: > > > > Hi Taehee, sorry for the late reply. I was out on vacation and needed > > to catch up on some stuff when I got back. > > Hi Mina, > Thank you so much for your review :) > > > > > On Tue, Oct 22, 2024 at 9:25 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 > > > header-data-split-thresh value should be 0. > > > > > > Tested-by: Stanislav Fomichev <sdf@fomichev.me> > > > Signed-off-by: Taehee Yoo <ap420073@gmail.com> > > > --- > > > > > > v4: > > > - Check condition before __netif_get_rx_queue(). > > > - Separate condition check. > > > - Add Test tag from Stanislav. > > > > > > 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..3425e872e87a 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; > > > @@ -140,6 +144,20 @@ int net_devmem_bind_dmabuf_to_queue(struct net_device *dev, u32 rxq_idx, > > > return -ERANGE; > > > } > > > > > > + if (!dev->ethtool_ops->get_ringparam) > > > + return -EOPNOTSUPP; > > > + > > > > Maybe an error code not EOPNOTSUPP. I think that gets returned when > > NET_DEVMEM is not compiled in and other situations like that. Lets > > pick another error code? Maybe ENODEV. > > There are several same code in the ethtool core. > It returns EOPNOTSUPP consistently. > In the v3 series, Brett reviewed it. > So, I changed it from EINVAL to EOPNOTSUPP it was reasonable to me. > So I prefer EOPNOTSUPP but I will follow your decision. > What do you think? > > > > > Also consider extack error message. But it's very unlikely to hit this > > error, so maybe not necessary. > > I removed extack from the v3. because ethtool doesn't use extack for > the same logic. It was reasonable to me. > Ah, looks like I accidentally re-opened discussion on a couple of items that you're already aligned on. Not critical. This is fine by me. > > > > > + dev->ethtool_ops->get_ringparam(dev, &ringparam, &kernel_ringparam, > > > + extack); > > > + if (kernel_ringparam.tcp_data_split != ETHTOOL_TCP_DATA_SPLIT_ENABLED) { > > > + NL_SET_ERR_MSG(extack, "tcp-data-split is disabled"); > > > + return -EINVAL; > > > + } > > > + if (kernel_ringparam.hds_thresh) { > > > + NL_SET_ERR_MSG(extack, "header-data-split-thresh is not zero"); > > > + return -EINVAL; > > > + } > > > + > > > > Thinking about drivers that support tcp-data-split, but don't support > > any kind of hds_thresh. For us (GVE), the hds_thresh is implicitly > > always 0. > > > > Does the driver need to explicitly set hds_thresh to 0? If so, that > > adds a bit of churn to driver code. Is it possible to detect in this > > function that the driver doesn't support hds_thresh and allow the > > binding if so? > > > > I see in the previous patch you do something like: > > > > supported_ring_params & ETHTOOL_RING_USE_HDS_THRS > > > > To detect there is hds_thresh support. I was wondering if something > > like this is possible so we don't have to update GVE and all future > > drivers to explicitly set thresh to 0. > > How about setting maximum hds_threshold to 0? > The default value of hds_threshold of course 0. > I think gve code does not need to change much, just adding like below > will be okay. > > I think if drivers don't support setting hds_threshold explicitly, it > is actually the same as support only 0. > So, there is no problem I think. > > I didn't analyze gve driver code, So I might think it too optimistically. > > #define GVE_HDS_THRESHOLD_MAX 0 > kernel_ering->hds_thresh = GVE_HDS_THRESHOLD_MAX; > kernel_ering->hds_thresh_max = GVE_HDS_THRESHOLD_MAX; > ... > .supported_ring_params = ETHTOOL_RING_USE_TCP_DATA_SPLIT | > ETHTOOL_RING_USE_HDS_THRS, > OK, if you can think of a way to do this without any churn to other drivers, that would be better, but this is fine by me either way. Reviewed-by: Mina Almasry <almasrymina@google.com>
diff --git a/net/core/devmem.c b/net/core/devmem.c index 11b91c12ee11..3425e872e87a 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; @@ -140,6 +144,20 @@ int net_devmem_bind_dmabuf_to_queue(struct net_device *dev, u32 rxq_idx, return -ERANGE; } + if (!dev->ethtool_ops->get_ringparam) + return -EOPNOTSUPP; + + dev->ethtool_ops->get_ringparam(dev, &ringparam, &kernel_ringparam, + extack); + if (kernel_ringparam.tcp_data_split != ETHTOOL_TCP_DATA_SPLIT_ENABLED) { + NL_SET_ERR_MSG(extack, "tcp-data-split is disabled"); + return -EINVAL; + } + if (kernel_ringparam.hds_thresh) { + NL_SET_ERR_MSG(extack, "header-data-split-thresh is not zero"); + return -EINVAL; + } + rxq = __netif_get_rx_queue(dev, rxq_idx); if (rxq->mp_params.mp_priv) { NL_SET_ERR_MSG(extack, "designated queue already memory provider bound");