| Message ID | 168675124698.2279.15699248221119454150.stgit@manet.1015granger.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Handle ARPHRD_NONE devices for siw | expand |
On 6/14/2023 10:00 AM, Chuck Lever wrote: > From: Chuck Lever <chuck.lever@oracle.com> > > Have the iwarp side properly set the ndev in the device's sgid_attrs > so that address resolution can treat it more like a RoCE device. > > Suggested-by: Jason Gunthorpe <jgg@nvidia.com> > Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > --- > drivers/infiniband/core/cache.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c > index 2e91d8879326..717524fe8a39 100644 > --- a/drivers/infiniband/core/cache.c > +++ b/drivers/infiniband/core/cache.c > @@ -1439,6 +1439,7 @@ static int config_non_roce_gid_cache(struct ib_device *device, > { > struct ib_gid_attr gid_attr = {}; > struct ib_gid_table *table; > + struct net_device *ndev; > int ret = 0; > int i; > > @@ -1457,10 +1458,21 @@ static int config_non_roce_gid_cache(struct ib_device *device, > i); > goto err; > } > + > + ndev = NULL; > + if (rdma_protocol_iwarp(device, port)) { > + ndev = ib_device_get_netdev(device, port); > + if (!ndev) > + continue; > + RCU_INIT_POINTER(gid_attr.ndev, ndev); > + } > + > gid_attr.index = i; > tprops->subnet_prefix = > be64_to_cpu(gid_attr.gid.global.subnet_prefix); > add_modify_gid(table, &gid_attr); > + > + dev_put(ndev); I'm not sure about two things here... 1) is it correct to dev_put(ndev) when returning? It seems that this may risk the device may vanish when it's still present in the cache. Feel free to tell me I'm confused. 2) Assuming yes, shouldn't the dev_put(ndev) move to within the new if(iwarp) block just above? Tom. > } > err: > mutex_unlock(&table->lock); > > >
> On Jun 23, 2023, at 2:37 PM, Tom Talpey <tom@talpey.com> wrote: > > On 6/14/2023 10:00 AM, Chuck Lever wrote: >> From: Chuck Lever <chuck.lever@oracle.com> >> Have the iwarp side properly set the ndev in the device's sgid_attrs >> so that address resolution can treat it more like a RoCE device. >> Suggested-by: Jason Gunthorpe <jgg@nvidia.com> >> Signed-off-by: Chuck Lever <chuck.lever@oracle.com> >> --- >> drivers/infiniband/core/cache.c | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c >> index 2e91d8879326..717524fe8a39 100644 >> --- a/drivers/infiniband/core/cache.c >> +++ b/drivers/infiniband/core/cache.c >> @@ -1439,6 +1439,7 @@ static int config_non_roce_gid_cache(struct ib_device *device, >> { >> struct ib_gid_attr gid_attr = {}; >> struct ib_gid_table *table; >> + struct net_device *ndev; >> int ret = 0; >> int i; >> @@ -1457,10 +1458,21 @@ static int config_non_roce_gid_cache(struct ib_device *device, >> i); >> goto err; >> } >> + >> + ndev = NULL; >> + if (rdma_protocol_iwarp(device, port)) { >> + ndev = ib_device_get_netdev(device, port); >> + if (!ndev) >> + continue; >> + RCU_INIT_POINTER(gid_attr.ndev, ndev); >> + } >> + >> gid_attr.index = i; >> tprops->subnet_prefix = >> be64_to_cpu(gid_attr.gid.global.subnet_prefix); >> add_modify_gid(table, &gid_attr); >> + >> + dev_put(ndev); > > I'm not sure about two things here... > > 1) is it correct to dev_put(ndev) when returning? It seems that this > may risk the device may vanish when it's still present in the cache. > Feel free to tell me I'm confused. ib_device_get_netdev() increments the ndev's reference count before returning. dev_put() releases that reference. > 2) Assuming yes, shouldn't the dev_put(ndev) move to within the new > if(iwarp) block just above? If the "if (iwarp)" block isn't taken, then ndev is NULL and that makes the dev_put() a no-op. > Tom. > >> } >> err: >> mutex_unlock(&table->lock); -- Chuck Lever
On 6/23/2023 2:40 PM, Chuck Lever III wrote: > > >> On Jun 23, 2023, at 2:37 PM, Tom Talpey <tom@talpey.com> wrote: >> >> On 6/14/2023 10:00 AM, Chuck Lever wrote: >>> From: Chuck Lever <chuck.lever@oracle.com> >>> Have the iwarp side properly set the ndev in the device's sgid_attrs >>> so that address resolution can treat it more like a RoCE device. >>> Suggested-by: Jason Gunthorpe <jgg@nvidia.com> >>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com> >>> --- >>> drivers/infiniband/core/cache.c | 12 ++++++++++++ >>> 1 file changed, 12 insertions(+) >>> diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c >>> index 2e91d8879326..717524fe8a39 100644 >>> --- a/drivers/infiniband/core/cache.c >>> +++ b/drivers/infiniband/core/cache.c >>> @@ -1439,6 +1439,7 @@ static int config_non_roce_gid_cache(struct ib_device *device, >>> { >>> struct ib_gid_attr gid_attr = {}; >>> struct ib_gid_table *table; >>> + struct net_device *ndev; >>> int ret = 0; >>> int i; >>> @@ -1457,10 +1458,21 @@ static int config_non_roce_gid_cache(struct ib_device *device, >>> i); >>> goto err; >>> } >>> + >>> + ndev = NULL; >>> + if (rdma_protocol_iwarp(device, port)) { >>> + ndev = ib_device_get_netdev(device, port); >>> + if (!ndev) >>> + continue; >>> + RCU_INIT_POINTER(gid_attr.ndev, ndev); >>> + } >>> + >>> gid_attr.index = i; >>> tprops->subnet_prefix = >>> be64_to_cpu(gid_attr.gid.global.subnet_prefix); >>> add_modify_gid(table, &gid_attr); >>> + >>> + dev_put(ndev); >> >> I'm not sure about two things here... >> >> 1) is it correct to dev_put(ndev) when returning? It seems that this >> may risk the device may vanish when it's still present in the cache. >> Feel free to tell me I'm confused. > > ib_device_get_netdev() increments the ndev's reference count > before returning. dev_put() releases that reference. > > >> 2) Assuming yes, shouldn't the dev_put(ndev) move to within the new >> if(iwarp) block just above? > > If the "if (iwarp)" block isn't taken, then ndev is NULL and that > makes the dev_put() a no-op. I still think it would be clearer and less side-effect sensitive to put the call inside the if(iwarp) block. Thanks for the dev_put clarification. Reviewed-by: Tom Talpey <tom@talpey.com> > > >> Tom. >> >>> } >>> err: >>> mutex_unlock(&table->lock); > > > -- > Chuck Lever > > >
diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c index 2e91d8879326..717524fe8a39 100644 --- a/drivers/infiniband/core/cache.c +++ b/drivers/infiniband/core/cache.c @@ -1439,6 +1439,7 @@ static int config_non_roce_gid_cache(struct ib_device *device, { struct ib_gid_attr gid_attr = {}; struct ib_gid_table *table; + struct net_device *ndev; int ret = 0; int i; @@ -1457,10 +1458,21 @@ static int config_non_roce_gid_cache(struct ib_device *device, i); goto err; } + + ndev = NULL; + if (rdma_protocol_iwarp(device, port)) { + ndev = ib_device_get_netdev(device, port); + if (!ndev) + continue; + RCU_INIT_POINTER(gid_attr.ndev, ndev); + } + gid_attr.index = i; tprops->subnet_prefix = be64_to_cpu(gid_attr.gid.global.subnet_prefix); add_modify_gid(table, &gid_attr); + + dev_put(ndev); } err: mutex_unlock(&table->lock);