Re: [PATCH rdma-next 3/4] RDMA/core: Introduce new GID table query API

[Jason Gunthorpe <jgg@nvidia.com>]

On Sun, Sep 13, 2020 at 11:02:33AM +0300, Leon Romanovsky wrote:
> On Fri, Sep 11, 2020 at 04:52:21PM -0300, Jason Gunthorpe wrote:
> > On Thu, Sep 10, 2020 at 05:22:03PM +0300, Leon Romanovsky wrote:
> > > From: Avihai Horon <avihaih@nvidia.com>
> > >
> > > Introduce rdma_query_gid_table which enables querying all the GID tables
> > > of a given device and copying the attributes of all valid GID entries to
> > > a provided buffer.
> > >
> > > This API provides a faster way to query a GID table using single call and
> > > will be used in libibverbs to improve current approach that requires
> > > multiple calls to open, close and read multiple sysfs files for a single
> > > GID table entry.
> > >
> > > Signed-off-by: Avihai Horon <avihaih@nvidia.com>
> > > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> > >  drivers/infiniband/core/cache.c         | 93 +++++++++++++++++++++++++
> > >  include/rdma/ib_cache.h                 |  5 ++
> > >  include/uapi/rdma/ib_user_ioctl_verbs.h |  8 +++
> > >  3 files changed, 106 insertions(+)
> > >
> > > diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c
> > > index cf49ac0b0aa6..175e229eccd3 100644
> > > +++ b/drivers/infiniband/core/cache.c
> > > @@ -1247,6 +1247,99 @@ rdma_get_gid_attr(struct ib_device *device, u8 port_num, int index)
> > >  }
> > >  EXPORT_SYMBOL(rdma_get_gid_attr);
> > >
> > > +/**
> > > + * rdma_get_ndev_ifindex - Reads ndev ifindex of the given gid attr.
> > > + * @gid_attr: Pointer to the GID attribute.
> > > + * @ndev_ifindex: Pointer through which the ndev ifindex is returned.
> > > + *
> > > + * Returns 0 on success or appropriate error code. The netdevice must be in UP
> > > + * state.
> > > + */
> > > +int rdma_get_ndev_ifindex(const struct ib_gid_attr *gid_attr, u32 *ndev_ifindex)
> > > +{
> > > +	struct net_device *ndev;
> > > +	int ret = 0;
> > > +
> > > +	if (rdma_protocol_ib(gid_attr->device, gid_attr->port_num)) {
> > > +		*ndev_ifindex = 0;
> > > +		return 0;
> > > +	}
> > > +
> > > +	rcu_read_lock();
> > > +	ndev = rcu_dereference(gid_attr->ndev);
> > > +	if (!ndev || (READ_ONCE(ndev->flags) & IFF_UP) == 0) {
> > > +		ret = -ENODEV;
> > > +		goto out;
> > > +	}
> >
> > None of this is necessary to read the ifindex, especially since the
> > read_lock is being held.
> 
> I see same rcu_read_lock->rcu_dereference->rcu_read_unlock pattern in
> rdma_read_gid_l2_fields(), why this function is different?

It doesn't hold the read_lock so it is using rcu to access
attr->ndev. With the read_lock held attr->ndev is stable and none of
this is needed.

Jason