Re: [PATCH for-next] IB/core: Only update PKEY and GID caches on respective events

[Leon Romanovsky <leon@kernel.org>]

On Thu, May 06, 2021 at 07:43:38AM +0000, Haakon Bugge wrote:
> 
> 
> > On 6 May 2021, at 09:29, Leon Romanovsky <leon@kernel.org> wrote:
> > 
> > On Tue, May 04, 2021 at 01:46:24PM +0200, Håkon Bugge wrote:
> >> Both the PKEY and GID tables in an HCA can hold in the order of
> >> hundreds entries. Reading them are expensive. Partly because the API
> >> for retrieving them only returns a single entry at a time. Further, on
> >> certain implementations, e.g., CX-3, the VFs are paravirtualized in
> >> this respect and have to rely on the PF driver to perform the
> >> read. This again demands VF to PF communication.
> >> 
> >> IB Core's cache is refreshed on all events. Hence, filter the refresh
> >> of the PKEY and GID caches based on the event received being
> >> IB_EVENT_PKEY_CHANGE and IB_EVENT_GID_CHANGE respectively.
> >> 
> >> Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
> >> ---
> >> drivers/infiniband/core/cache.c | 25 ++++++++++++++++---------
> >> 1 file changed, 16 insertions(+), 9 deletions(-)
> >> 
> >> diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c
> >> index 5c9fac7..531ae6b 100644
> >> --- a/drivers/infiniband/core/cache.c
> >> +++ b/drivers/infiniband/core/cache.c
> >> @@ -1472,10 +1472,14 @@ static int config_non_roce_gid_cache(struct ib_device *device,
> >> }
> >> 
> >> static int
> >> -ib_cache_update(struct ib_device *device, u8 port, bool enforce_security)
> >> +ib_cache_update(struct ib_device *device, u8 port, enum ib_event_type event,
> >> +		bool reg_dev, bool enforce_security)
> >> {
> >> 	struct ib_port_attr       *tprops = NULL;
> >> -	struct ib_pkey_cache      *pkey_cache = NULL, *old_pkey_cache;
> >> +	struct ib_pkey_cache      *pkey_cache = NULL;
> >> +	struct ib_pkey_cache      *old_pkey_cache = NULL;
> >> +	bool			   update_pkey_cache = (reg_dev || event == IB_EVENT_PKEY_CHANGE);
> >> +	bool			   update_gid_cache = (reg_dev || event == IB_EVENT_GID_CHANGE);
> > 
> > I'm not super excited to see "events" in this function and would be more
> > happy to see it is handled in ib_cache_event_task() while the function
> > signature will be:
> > ib_cache_update(struct ib_device *device, u8 port, bool all_gids, bool
> > all_pkeys, bool enforce_security)
> 
> I was thinking the other way around; if a new entity, FOO, gets cached, to be updated on IB_EVENT_FOO_CHANGE, the change to support this would be more contained; you only need to change ib_cache_update().

We are already handling enforce_security outside of ib_cache_update().

> 
> But by all means, will send a v2 based on you recommendation.

Thanks

> 
> 
> Thxs, Håkon
> 
> 
> 
> > 
> > Thanks
>