| Message ID | 1468827648-17275-1-git-send-email-yuval.shaia@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, Jul 18, 2016 at 12:40:48AM -0700, Yuval Shaia wrote: > Decouple SG support from HW ability to do UD checksum. > This coupling is for historical reasons and removed with 'commit > ec5f06156423 ("net: Kill link between CSUM and SG features.")' > > Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com> > --- > v0 -> v1: > * Fix patch title name > * Move settings to ipoib_setup instead of ipoib_set_dev_features Thanks.
On Mon, Jul 18, 2016 at 12:40:48AM -0700, Yuval Shaia wrote: > Decouple SG support from HW ability to do UD checksum. > This coupling is for historical reasons and removed with 'commit > ec5f06156423 ("net: Kill link between CSUM and SG features.")' Shouldn't this also check that the QP's sg limit is something appropriate? At least > 1 .. Presumably things still work if the skb needs more sg's than are available? Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 18, 2016 at 11:19:21AM -0600, Jason Gunthorpe wrote: > On Mon, Jul 18, 2016 at 12:40:48AM -0700, Yuval Shaia wrote: > > Decouple SG support from HW ability to do UD checksum. > > This coupling is for historical reasons and removed with 'commit > > ec5f06156423 ("net: Kill link between CSUM and SG features.")' > > Shouldn't this also check that the QP's sg limit is something > appropriate? At least > 1 .. > > Presumably things still work if the skb needs more sg's than are > available? IMHO, this question is relevant for the code before change as well. > > Jason > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 18, 2016 at 11:19:21AM -0600, Jason Gunthorpe wrote: > On Mon, Jul 18, 2016 at 12:40:48AM -0700, Yuval Shaia wrote: > > Decouple SG support from HW ability to do UD checksum. > > This coupling is for historical reasons and removed with 'commit > > ec5f06156423 ("net: Kill link between CSUM and SG features.")' > > Shouldn't this also check that the QP's sg limit is something > appropriate? At least > 1 .. What is special with 1? Network stack can pass MAX_SKB_FRAGS frags so even if QP's SG limit is 3 we are still in problem. Fortunately this was addressed in 'commit 78a50a5 ("Add handling for sending of skb with many frags"). > > Presumably things still work if the skb needs more sg's than are > available? > > Jason > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 19, 2016 at 03:55:57PM +0300, Yuval Shaia wrote: > On Mon, Jul 18, 2016 at 11:19:21AM -0600, Jason Gunthorpe wrote: > > On Mon, Jul 18, 2016 at 12:40:48AM -0700, Yuval Shaia wrote: > > > Decouple SG support from HW ability to do UD checksum. > > > This coupling is for historical reasons and removed with 'commit > > > ec5f06156423 ("net: Kill link between CSUM and SG features.")' > > > > Shouldn't this also check that the QP's sg limit is something > > appropriate? At least > 1 .. > > What is special with 1? One means the hardware doesn't support SG at all. Is there a reason to sg_linearize down to ipoib if we know the hardware doesn't support SG at all? Presumably it is better to let the netstack deal with that case. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 19, 2016 at 01:34:53PM -0600, Jason Gunthorpe wrote: > On Tue, Jul 19, 2016 at 03:55:57PM +0300, Yuval Shaia wrote: > > On Mon, Jul 18, 2016 at 11:19:21AM -0600, Jason Gunthorpe wrote: > > > On Mon, Jul 18, 2016 at 12:40:48AM -0700, Yuval Shaia wrote: > > > > Decouple SG support from HW ability to do UD checksum. > > > > This coupling is for historical reasons and removed with 'commit > > > > ec5f06156423 ("net: Kill link between CSUM and SG features.")' > > > > > > Shouldn't this also check that the QP's sg limit is something > > > appropriate? At least > 1 .. > > > > What is special with 1? > > One means the hardware doesn't support SG at all. > > Is there a reason to sg_linearize down to ipoib if we know the > hardware doesn't support SG at all? Presumably it is better to let the > netstack deal with that case. ok, i see the point but as Leon Romanovsky mentioned - this is not an issue with the patch itself, right? i.e. the original code relays on the fact that HW supports UD-CSUM and did not checked QP's sg limit (we even do not have any QP at this stage yet). > > Jason > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 19, 2016 at 11:02:32PM +0300, Yuval Shaia wrote: > ok, i see the point but as Leon Romanovsky mentioned - this is not an issue > with the patch itself, right? i.e. the original code relays on the fact > that HW supports UD-CSUM and did not checked QP's sg limit (we even do not > have any QP at this stage yet). ud-csum was only supported by one IB vendor and that vendor supports SG. You are changing things to enable SG on hardware that has never had SG enabled, so you need to make sure it works right.. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 19, 2016 at 02:11:13PM -0600, Jason Gunthorpe wrote: > On Tue, Jul 19, 2016 at 11:02:32PM +0300, Yuval Shaia wrote: > > > ok, i see the point but as Leon Romanovsky mentioned - this is not an issue > > with the patch itself, right? i.e. the original code relays on the fact > > that HW supports UD-CSUM and did not checked QP's sg limit (we even do not > > have any QP at this stage yet). > > ud-csum was only supported by one IB vendor and that vendor supports > SG. > > You are changing things to enable SG on hardware that has never had SG > enabled, so you need to make sure it works right.. My thoughts exactly. Ira -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Sep 01, 2016 at 09:10:22PM -0400, ira.weiny wrote: > On Tue, Jul 19, 2016 at 02:11:13PM -0600, Jason Gunthorpe wrote: > > On Tue, Jul 19, 2016 at 11:02:32PM +0300, Yuval Shaia wrote: > > > > > ok, i see the point but as Leon Romanovsky mentioned - this is not an issue > > > with the patch itself, right? i.e. the original code relays on the fact > > > that HW supports UD-CSUM and did not checked QP's sg limit (we even do not > > > have any QP at this stage yet). > > > > ud-csum was only supported by one IB vendor and that vendor supports > > SG. > > > > You are changing things to enable SG on hardware that has never had SG > > enabled, so you need to make sure it works right.. > > My thoughts exactly. > Ira Already posted v2. > -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c index 5f58c41..de7b010 100644 --- a/drivers/infiniband/ulp/ipoib/ipoib_main.c +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c @@ -1763,8 +1763,8 @@ void ipoib_setup(struct net_device *dev) dev->addr_len = INFINIBAND_ALEN; dev->type = ARPHRD_INFINIBAND; dev->tx_queue_len = ipoib_sendq_size * 2; - dev->features = (NETIF_F_VLAN_CHALLENGED | - NETIF_F_HIGHDMA); + dev->features = NETIF_F_VLAN_CHALLENGED | NETIF_F_HIGHDMA | + NETIF_F_SG; netif_keep_dst(dev); memcpy(dev->broadcast, ipv4_bcast_addr, INFINIBAND_ALEN); @@ -1967,8 +1967,7 @@ int ipoib_set_dev_features(struct ipoib_dev_priv *priv, struct ib_device *hca) priv->hca_caps = hca->attrs.device_cap_flags; if (priv->hca_caps & IB_DEVICE_UD_IP_CSUM) { - priv->dev->hw_features = NETIF_F_SG | - NETIF_F_IP_CSUM | NETIF_F_RXCSUM; + priv->dev->hw_features = NETIF_F_IP_CSUM | NETIF_F_RXCSUM; if (priv->hca_caps & IB_DEVICE_UD_TSO) priv->dev->hw_features |= NETIF_F_TSO;
Decouple SG support from HW ability to do UD checksum. This coupling is for historical reasons and removed with 'commit ec5f06156423 ("net: Kill link between CSUM and SG features.")' Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com> --- v0 -> v1: * Fix patch title name * Move settings to ipoib_setup instead of ipoib_set_dev_features --- drivers/infiniband/ulp/ipoib/ipoib_main.c | 7 +++---- 1 files changed, 3 insertions(+), 4 deletions(-)