[ofa-general] Re: [PATCH] [RFC] IB/cache: Add ib_cache report for cache in process

[Michael S. Tsirkin]

> Quoting Moni Levy <myopenib at gmail.com>:
> Subject: [PATCH] [RFC] IB/cache: Add ib_cache report for cache in process
> 
> Content-Type: text/plain; charset=ISO-8859-1
> Content-Transfer-Encoding: 7bit
> X-Spam: exempt
> X-MAIL-FROM: <myopenib at gmail.com>
> X-SOURCE-IP: [64.233.182.184]
> Status:   
> X-OriginalArrivalTime: 29 Mar 2007 14:42:01.0018 (UTC) FILETIME=[6902C1A0:01C77210]
> 
> We discovered a race between calls of ib_get_cached_pkey & ib_find_cached_pkey triggered by a PKEY_CHANGE event and the update of the cache they read from.
> 
> The new return code (-ESTALE) informs the callers to ib_get_cached_pkey and ib_find_cached_pkey that the ib_cache is in process of updating itself and that the call should be retried if an up to date information is needed. 
> 
> Signed-off-by: Moni Levy <monil at voltaire.com>

OK, but we still need the code to make ULPs retry failed cache queries,
right?

> ---
>  drivers/infiniband/core/cache.c |   11 +++++++++++
>  include/rdma/ib_verbs.h         |    1 +
>  2 files changed, 12 insertions(+)
> 
> Index: b/include/rdma/ib_verbs.h
> ===================================================================
> --- a/include/rdma/ib_verbs.h	2007-03-29 08:11:37.544251082 +0200
> +++ b/include/rdma/ib_verbs.h	2007-03-29 08:11:58.606496898 +0200
> @@ -849,6 +849,7 @@ struct ib_cache {
>  	struct ib_pkey_cache  **pkey_cache;
>  	struct ib_gid_cache   **gid_cache;
>  	u8                     *lmc_cache;
> +	atomic_t		coherent;
>  };
>  
>  struct ib_dma_mapping_ops {

What if the coherent flag is changed while reader is running?
Might not be an issue, but still somewhat messy.

We are taking the cache lock anyway - can't we make it a regular integer,
and set it under write lock?
The case of access to cache that is consistent is probably what we want
to optimize for.

> Index: b/drivers/infiniband/core/cache.c
> ===================================================================
> --- a/drivers/infiniband/core/cache.c	2007-03-29 08:11:37.583244132 +0200
> +++ b/drivers/infiniband/core/cache.c	2007-03-29 11:32:38.915910966 +0200
> @@ -38,6 +38,7 @@
>  #include <linux/module.h>
>  #include <linux/errno.h>
>  #include <linux/slab.h>
> +#include <asm/atomic.h>
>  
>  #include <rdma/ib_cache.h>
>  
> @@ -141,6 +142,9 @@ int ib_get_cached_pkey(struct ib_device 
>  	unsigned long flags;
>  	int ret = 0;
>  
> +	if (!atomic_read(&device->cache.coherent))
> +		return -ESTALE;
> +

Should be unlikely() I guess?

>  	if (port_num < start_port(device) || port_num > end_port(device))
>  		return -EINVAL;
>  
> @@ -169,6 +173,9 @@ int ib_find_cached_pkey(struct ib_device
>  	int i;
>  	int ret = -ENOENT;
>  
> +	if (!atomic_read(&device->cache.coherent))
> +		return -ESTALE;
> +
>  	if (port_num < start_port(device) || port_num > end_port(device))
>  		return -EINVAL;
>  
> @@ -273,6 +280,8 @@ static void ib_cache_update(struct ib_de
>  
>  	write_unlock_irq(&device->cache.lock);
>  
> +	atomic_set(&device->cache.coherent, 1);
> +
>  	kfree(old_pkey_cache);
>  	kfree(old_gid_cache);
>  	kfree(tprops);

So let's move this 2 lines up and it'll be under lock. 

> @@ -306,6 +315,7 @@ static void ib_cache_event(struct ib_eve
>  	    event->event == IB_EVENT_CLIENT_REREGISTER) {
>  		work = kmalloc(sizeof *work, GFP_ATOMIC);
>  		if (work) {
> +			atomic_set(&work->device->cache.coherent, 0);
>  			INIT_WORK(&work->work, ib_cache_task);
>  			work->device   = event->device;
>  			work->port_num = event->element.port_num;

And here, take the write lock and clear the flag.

> @@ -319,6 +329,7 @@ static void ib_cache_setup_one(struct ib
>  	int p;
>  
>  	rwlock_init(&device->cache.lock);
> +	atomic_set(&device->cache.coherent, 0);
>  
>  	device->cache.pkey_cache =
>  		kmalloc(sizeof *device->cache.pkey_cache *


-- 
MST