[PATCH 00/11] New Caching mechanism for ib_core

[PATCH 00/11] New Caching mechanism for ib_core

[Goldwyn Rodrigues]

Hi,

This series of patches introduces caching as a library in the ib_core 
as compared to the previous of implementation as a part of the
ib_device calls. Each driver can make use of this by including
the ib_cache data structure in the individual device structure.
The updation of the cache is the responsibility of the driver, and
is best incorporated in the pkey/gid query functions. However, the
driver may choose to pre-populate the cache on initialization.

The main motivations are:

1. Greater degree of control by individual drivers. Drivers have a
   choice to use it or not.
2. The library functions do not sleep, and can be called from any context.

In the bargain we lose the lmc cache. However, if we move it to the device
structure like the pkey_tbl_len, it can be accessed directly (TODO).
Let me know what you think about it.

I have changed only the mthca driver for now to use this API of the
cache. If I get positive comments, I will incorporate it in the
rest of the drivers as well. Let me know what you think.

TODO:
Use finer granuality of locking for pkey and gid tables.

Suggestions welcome.

-- 
Goldwyn
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 00/11] New Caching mechanism for ib_core

[Hefty, Sean]

> The main motivations are:
> 
> 1. Greater degree of control by individual drivers. Drivers have a
>    choice to use it or not.

I believe that some callers need to know that specific query calls will not sleep.  That capability should either be required or exposed through the API.

> 2. The library functions do not sleep, and can be called from any context.
> 
> In the bargain we lose the lmc cache. However, if we move it to the device
> structure like the pkey_tbl_len, it can be accessed directly (TODO).
> Let me know what you think about it.

The LMC cache is accessed by the MAD layer.  I know that MADs aren't considered a performance path, but we don't want to query the device for every MAD.

- Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/11] New Caching mechanism for ib_core

[Goldwyn Rodrigues]

Thanks Sean for the review.

On Tue, Nov 08, 2011 at 11:46:50PM +0000, Hefty, Sean wrote:
> > The main motivations are:
> > 
> > 1. Greater degree of control by individual drivers. Drivers have a
> >    choice to use it or not.
> 
> I believe that some callers need to know that specific query calls will not sleep.  That capability should either be required or exposed through the API.

The new cache access functions do not sleep. This is the primary
objective of the exercise. See motviation 2 :) 
Exceptions are the init functions which may sleep because of kzalloc().
Did you want me to mention this specifically in the comments?

> 
> > 2. The library functions do not sleep, and can be called from any context.
> > 
> > In the bargain we lose the lmc cache. However, if we move it to the device
> > structure like the pkey_tbl_len, it can be accessed directly (TODO).
> > Let me know what you think about it.
> 
> The LMC cache is accessed by the MAD layer.  I know that MADs aren't considered a performance path, but we don't want to query the device for every MAD.
> 

I understand. I will try to incorporate the lmc cache in the device
structure while I wait for the rest of the comments.

Regards,

-- 
Goldwyn
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 00/11] New Caching mechanism for ib_core

[Hefty, Sean]

> > > 1. Greater degree of control by individual drivers. Drivers have a
> > >    choice to use it or not.
> >
> > I believe that some callers need to know that specific query calls will not
> sleep.  That capability should either be required or exposed through the API.
> 
> The new cache access functions do not sleep. This is the primary
> objective of the exercise. See motviation 2 :)
> Exceptions are the init functions which may sleep because of kzalloc().
> Did you want me to mention this specifically in the comments?

Yes, but the use of the cache is hidden from the user.  Currently calls like ib_query_gid() are allowed to sleep.  If drivers are given the choice whether or not to use the cache, then there's no way for a user to know whether a query call will sleep or not.

- Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/11] New Caching mechanism for ib_core

[Goldwyn Rodrigues]

On Wed, Nov 09, 2011 at 03:12:04PM +0000, Hefty, Sean wrote:
> > > > 1. Greater degree of control by individual drivers. Drivers have a
> > > >    choice to use it or not.
> > >
> > > I believe that some callers need to know that specific query calls will not
> > sleep.  That capability should either be required or exposed through the API.
> > 
> > The new cache access functions do not sleep. This is the primary
> > objective of the exercise. See motviation 2 :)
> > Exceptions are the init functions which may sleep because of kzalloc().
> > Did you want me to mention this specifically in the comments?
> 
> Yes, but the use of the cache is hidden from the user.  

user who?

> Currently calls like ib_query_gid() are allowed to sleep.  If drivers are given the choice whether or not to use the cache, then there's no way for a user to know whether a query call will sleep or not.
> 

The cache calls do not sleep. Period.
AFAIK, the problem is with assuming calls do not sleep, when they
actually do and not vice versa.

What problems do you forsee if we declare that cache calls do not sleep?

Since the drivers would be users of cache calls, the behavior would be
as assumed for the query calls for the driver. Refer mthca
implementation (PATCH 11/11): If the entry is found in cache, it
will return immediately, and will not sleep. If the entry is not
found, it will query the device and may sleep. In essence we still
hold that the query functions may sleep, but if the data is available
in cache it will return faster.


-- 
Goldwyn
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 00/11] New Caching mechanism for ib_core

[Hefty, Sean]

> > Yes, but the use of the cache is hidden from the user.
> 
> user who?

ib_cm, rdma_cm, ib_mad, ib_ipoib, etc.  You would need to trace the use up to all ULPs.

> Since the drivers would be users of cache calls, the behavior would be
> as assumed for the query calls for the driver. Refer mthca
> implementation (PATCH 11/11): If the entry is found in cache, it
> will return immediately, and will not sleep.

> If the entry is not found, it will query the device and may sleep.

This is the issue.  You changed calls from ib_find_cached_pkey() and ib_find_cached_gid() to ib_query_pkey() and ib_query_gid().  The latter calls may sleep, but I don't believe that we can sleep in all places where the new functions are used.  Changing from ib_find_cached_gid to ib_query_gid changes the locking restrictions up through the stack.  It doesn't matter if the cache doesn't block, the exposed calls may.

- Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/11] New Caching mechanism for ib_core

[Goldwyn Rodrigues]

Hi Sean,

On Thu, Nov 10, 2011 at 07:03:40AM +0000, Hefty, Sean wrote:
> > > Yes, but the use of the cache is hidden from the user.
> > 
> > user who?
> 
> ib_cm, rdma_cm, ib_mad, ib_ipoib, etc.  You would need to trace the use up to all ULPs.
> 
> > Since the drivers would be users of cache calls, the behavior would be
> > as assumed for the query calls for the driver. Refer mthca
> > implementation (PATCH 11/11): If the entry is found in cache, it
> > will return immediately, and will not sleep.
> 
> > If the entry is not found, it will query the device and may sleep.
> 
> This is the issue.  You changed calls from ib_find_cached_pkey() and ib_find_cached_gid() to ib_query_pkey() and ib_query_gid().  The latter calls may sleep, but I don't believe that we can sleep in all places where the new functions are used.  Changing from ib_find_cached_gid to ib_query_gid changes the locking restrictions up through the stack.  It doesn't matter if the cache doesn't block, the exposed calls may.

Thanks for explaining. I see your point now. This had been in the
orignal discussion, but I missed on this point while working
on this. I will redo the patches after reviewing the function usage.

-- 
Goldwyn
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html