> -----Original Message-----
> From: Jason Gunthorpe <jgg@xxxxxxxx>
> Sent: Friday, January 11, 2019 4:53 PM
> To: Leon Romanovsky <leon@xxxxxxxxxx>
> Cc: Doug Ledford <dledford@xxxxxxxxxx>; Leon Romanovsky
> <leonro@xxxxxxxxxxxx>; RDMA mailing list <linux-rdma@xxxxxxxxxxxxxxx>;
> Parav Pandit <parav@xxxxxxxxxxxx>
> Subject: Re: [PATCH rdma-next 3/7] RDMA/core: Introduce helper functions
> for cache cleanup, update
>
> On Tue, Dec 18, 2018 at 02:28:32PM +0200, Leon Romanovsky wrote:
> > From: Parav Pandit <parav@xxxxxxxxxxxx>
> >
> > Move code to reusable helper functions to reuse in subsequent patch in
> > handling net namespace change netlink command for an rdma device.
> >
> > Signed-off-by: Parav Pandit <parav@xxxxxxxxxxxx>
> > Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> > drivers/infiniband/core/cache.c | 25 +++++++++++++++----------
> > drivers/infiniband/core/core_priv.h | 3 +++
> > 2 files changed, 18 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/infiniband/core/cache.c
> > b/drivers/infiniband/core/cache.c index 7b04590f307f..ae67396af92f
> > 100644
> > +++ b/drivers/infiniband/core/cache.c
> > @@ -896,7 +896,7 @@ static int _gid_table_setup_one(struct ib_device
> *ib_dev)
> > return -ENOMEM;
> > }
> >
> > -static void gid_table_cleanup_one(struct ib_device *ib_dev)
> > +void ib_gid_table_cleanup_one(struct ib_device *ib_dev)
> > {
> > struct ib_gid_table *table;
> > u8 port;
> > @@ -906,6 +906,10 @@ static void gid_table_cleanup_one(struct
> ib_device *ib_dev)
> > cleanup_gid_table_port(ib_dev, port +
> rdma_start_port(ib_dev),
> > table);
> > }
> > + /*
> > + * Flush the wq for any pending GID delete work.
> > + */
> > + flush_workqueue(ib_wq);
> > }
> >
> > static int gid_table_setup_one(struct ib_device *ib_dev) @@ -1426,9
> > +1430,16 @@ static void ib_cache_event(struct ib_event_handler
> *handler,
> > }
> > }
> >
> > -int ib_cache_setup_one(struct ib_device *device)
> > +void ib_device_cache_update(struct ib_device *device)
> > {
> > int p;
> > +
> > + for (p = rdma_start_port(device); p <= rdma_end_port(device); p++)
> > + ib_cache_update(device, p, true);
> > +}
> > +
> > +int ib_cache_setup_one(struct ib_device *device) {
> > int err;
> >
> > rwlock_init(&device->cache.lock);
> > @@ -1447,8 +1458,7 @@ int ib_cache_setup_one(struct ib_device *device)
> > return err;
> > }
> >
> > - for (p = 0; p <= rdma_end_port(device) - rdma_start_port(device);
> ++p)
> > - ib_cache_update(device, p + rdma_start_port(device), true);
> > + ib_device_cache_update(device);
> >
> > INIT_IB_EVENT_HANDLER(&device->cache.event_handler,
> > device, ib_cache_event);
> > @@ -1484,10 +1494,5 @@ void ib_cache_cleanup_one(struct ib_device
> *device)
> > */
> > ib_unregister_event_handler(&device->cache.event_handler);
> > flush_workqueue(ib_wq);
> > - gid_table_cleanup_one(device);
> > -
> > - /*
> > - * Flush the wq second time for any pending GID delete work.
> > - */
> > - flush_workqueue(ib_wq);
> > + ib_gid_table_cleanup_one(device);
> > }
> > diff --git a/drivers/infiniband/core/core_priv.h
> > b/drivers/infiniband/core/core_priv.h
> > index f2da31de62c9..7dc6f814f387 100644
> > +++ b/drivers/infiniband/core/core_priv.h
> > @@ -357,4 +357,7 @@ int ib_setup_port_attrs(struct ib_core_device
> *coredev,
> > int (*port_callback)(struct ib_device *,
> > u8, struct kobject *),
> > bool alloc_hw_stats);
> > +
> > +void ib_device_cache_update(struct ib_device *device); void
> > +ib_gid_table_cleanup_one(struct ib_device *device);
>
> This is super goofy, why do we have a 'cleanup' followed by 'update'
> API design?
Cleanup removes all entries from the gid table.
It makes sure that all references drop.
It doesn't mandate to first move netdevice to net namespace or any such crazy sequence.
Than update() below rebuilds the table.
>
> ib_gid_table_cleanup_one(device);
> free_client_contexts(device);
> device->reg_state = IB_DEV_UNREGISTERED;
>
> device->netns_shared = 0;
> rdma_coredev_net_set(&device->coredev, net);
>
> rdma_roce_rescan_device(device);
> ib_device_cache_update(device);
>
> Makes no sense.
>
> Lets just have a ib_gid_cache_restart() API or something?
>
> I don't think this really needs two steps?
>
It is just cleaner by reusing some of the existing core functions in two steps.
It follows the regular unreg() reg() sequence.
So I think its ok.
