[ewg] Re: [PATCH] [RFC] IB/cache: Add ib_cache report for cache in process

[Moni Levy]

On 3/29/07, Michael S. Tsirkin <mst at dev.mellanox.co.il> wrote:
> > 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?

That does not seem trivial for all the ULPs. Can't we just assume that
-ESTALE would fail the ULPs instead of misleading them ? That way we
are not making anything behave
worse.

>
> > ---
> >  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?

Ok.

> The case of access to cache that is consistent is probably what we want
> to optimize for.

Right

>
> > 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?

Ok

>
> >       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.

Ok

>
> > @@ -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.

Ok

>
> > @@ -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
>