[PATCH -mm 1/6] slab: introduce __kfree_rcu

[PATCH -mm 1/6] slab: introduce __kfree_rcu

[Lai Jiangshan]

Introduce __kfree_rcu() for kfree_rcu()

We can calculate the object poiter from a poiter inside this
object in slab.c, so we can use a portion_to_obj() to instead
various container_of() for rcu callback and free the object.

Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
---
diff --git a/mm/slab.c b/mm/slab.c
index 4d00855..a067d3f 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -634,6 +634,17 @@ static inline unsigned int obj_to_index(const struct kmem_cache *cache,
 	return reciprocal_divide(offset, cache->reciprocal_buffer_size);
 }
 
+static inline void *portion_to_obj(void *portion)
+{
+	struct page *page = virt_to_head_page(portion);
+	struct slab *slab = page_get_slab(page);
+	struct kmem_cache *cache = page_get_cache(page);
+	unsigned int offset = portion - slab->s_mem;
+	unsigned int index = offset / cache->buffer_size;
+
+	return index_to_obj(cache, slab, index);
+}
+
 /*
  * These are the default caches for kmalloc. Custom caches can have other sizes.
  */
@@ -3728,6 +3739,17 @@ void kfree(const void *objp)
 }
 EXPORT_SYMBOL(kfree);
 
+static void kfree_rcu_callback(struct rcu_head *rcu)
+{
+	kfree(portion_to_obj(rcu));
+}
+
+void __kfree_rcu(const void *objp, struct rcu_head *rcu)
+{
+	call_rcu(rcu, kfree_rcu_callback);
+}
+EXPORT_SYMBOL(__kfree_rcu);
+
 unsigned int kmem_cache_size(struct kmem_cache *cachep)
 {
 	return obj_size(cachep);

Re: [PATCH -mm 1/6] slab: introduce __kfree_rcu

[Pekka Enberg]

On Tue, 2009-03-03 at 21:44 +0800, Lai Jiangshan wrote:
> Introduce __kfree_rcu() for kfree_rcu()
> 
> We can calculate the object poiter from a poiter inside this
> object in slab.c, so we can use a portion_to_obj() to instead
> various container_of() for rcu callback and free the object.
> 
> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
> ---
> diff --git a/mm/slab.c b/mm/slab.c
> index 4d00855..a067d3f 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -634,6 +634,17 @@ static inline unsigned int obj_to_index(const struct kmem_cache *cache,
>  	return reciprocal_divide(offset, cache->reciprocal_buffer_size);
>  }
>  
> +static inline void *portion_to_obj(void *portion)
> +{
> +	struct page *page = virt_to_head_page(portion);
> +	struct slab *slab = page_get_slab(page);
> +	struct kmem_cache *cache = page_get_cache(page);
> +	unsigned int offset = portion - slab->s_mem;
> +	unsigned int index = offset / cache->buffer_size;
> +
> +	return index_to_obj(cache, slab, index);
> +}

A minor nit: I think this would be more readable if you separated
variable declarations from the initializations. Also, you can probably
drop the inline from the function declaration and let GCC decide what to
do.

Re: [PATCH -mm 1/6] slab: introduce __kfree_rcu

[Christoph Lameter]

On Mon, 23 Mar 2009, Pekka Enberg wrote:

> > +static inline void *portion_to_obj(void *portion)
> > +{
> > +	struct page *page = virt_to_head_page(portion);
> > +	struct slab *slab = page_get_slab(page);
> > +	struct kmem_cache *cache = page_get_cache(page);
> > +	unsigned int offset = portion - slab->s_mem;
> > +	unsigned int index = offset / cache->buffer_size;
> > +
> > +	return index_to_obj(cache, slab, index);
> > +}
>
> A minor nit: I think this would be more readable if you separated
> variable declarations from the initializations. Also, you can probably
> drop the inline from the function declaration and let GCC decide what to
> do.

Thats debatable. I find the setting up a number of variables
that are all dependend in the above manner very readable. They are usually
repetitive. Multiple functions use similar initializations.

Re: [PATCH -mm 1/6] slab: introduce __kfree_rcu

[Ingo Molnar]

* Christoph Lameter <cl@linux-foundation.org> wrote:

> On Mon, 23 Mar 2009, Pekka Enberg wrote:
> 
> > > +static inline void *portion_to_obj(void *portion)
> > > +{
> > > +	struct page *page = virt_to_head_page(portion);
> > > +	struct slab *slab = page_get_slab(page);
> > > +	struct kmem_cache *cache = page_get_cache(page);
> > > +	unsigned int offset = portion - slab->s_mem;
> > > +	unsigned int index = offset / cache->buffer_size;
> > > +
> > > +	return index_to_obj(cache, slab, index);
> > > +}
> >
> > A minor nit: I think this would be more readable if you separated
> > variable declarations from the initializations. Also, you can probably
> > drop the inline from the function declaration and let GCC decide what to
> > do.
> 
> Thats debatable. I find the setting up a number of variables that 
> are all dependend in the above manner very readable. They are 
> usually repetitive. Multiple functions use similar 
> initializations.

I agree with Pekka, it's clearly more readable when separated out 
nicely:

	struct kmem_cache *cache;
	unsigned int offset;
	unsigned int index;
	struct page *page;
	struct slab *slab;

	page	= virt_to_head_page(portion);
	slab	= page_get_slab(page);
	cache	= page_get_cache(page);

	offset	= portion - slab->s_mem;
	index	= offset / cache->buffer_size;

The original form is hard to read due to lack of structure.

	Ingo

Re: [PATCH -mm 1/6] slab: introduce __kfree_rcu

[Christoph Lameter]

On Mon, 23 Mar 2009, Ingo Molnar wrote:

>
> * Christoph Lameter <cl@linux-foundation.org> wrote:
>
> > On Mon, 23 Mar 2009, Pekka Enberg wrote:
> >
> > > > +static inline void *portion_to_obj(void *portion)
> > > > +{
> > > > +	struct page *page = virt_to_head_page(portion);
> > > > +	struct slab *slab = page_get_slab(page);
> > > > +	struct kmem_cache *cache = page_get_cache(page);
> > > > +	unsigned int offset = portion - slab->s_mem;
> > > > +	unsigned int index = offset / cache->buffer_size;
> > > > +
> > > > +	return index_to_obj(cache, slab, index);
> > > > +}
> > >
> > > A minor nit: I think this would be more readable if you separated
> > > variable declarations from the initializations. Also, you can probably
> > > drop the inline from the function declaration and let GCC decide what to
> > > do.
> >
> > Thats debatable. I find the setting up a number of variables that
> > are all dependend in the above manner very readable. They are
> > usually repetitive. Multiple functions use similar
> > initializations.
>
> I agree with Pekka, it's clearly more readable when separated out
> nicely:
>
> 	struct kmem_cache *cache;
> 	unsigned int offset;
> 	unsigned int index;
> 	struct page *page;
> 	struct slab *slab;
>
> 	page	= virt_to_head_page(portion);
> 	slab	= page_get_slab(page);
> 	cache	= page_get_cache(page);
>
> 	offset	= portion - slab->s_mem;
> 	index	= offset / cache->buffer_size;
>
> The original form is hard to read due to lack of structure.

Structure can also be established differently:

static inline void *portion_to_obj(void *portion)
{
	struct page *page = virt_to_head_page(portion);
	struct slab *slab = page_get_slab(page);
	struct kmem_cache *cache = page_get_cache(page);

	unsigned int offset = portion - slab->s_mem;
	unsigned int index = offset / cache->buffer_size;

	return index_to_obj(cache, slab, index);
}

It would be good if the whole series of actions that need to be taken in
order for the function to "get to know" the slab the object parms would be
simpler. Like its done in ruby

	(page, slab, cache) = get_slab_info(portion)

	(offset, index) = get_position_info(slab, portion)

But how can this be done in C without weird pointer passing?

Re: [PATCH -mm 1/6] slab: introduce __kfree_rcu

[Ingo Molnar]

* Christoph Lameter <cl@linux-foundation.org> wrote:

> On Mon, 23 Mar 2009, Ingo Molnar wrote:
> 
> >
> > * Christoph Lameter <cl@linux-foundation.org> wrote:
> >
> > > On Mon, 23 Mar 2009, Pekka Enberg wrote:
> > >
> > > > > +static inline void *portion_to_obj(void *portion)
> > > > > +{
> > > > > +	struct page *page = virt_to_head_page(portion);
> > > > > +	struct slab *slab = page_get_slab(page);
> > > > > +	struct kmem_cache *cache = page_get_cache(page);
> > > > > +	unsigned int offset = portion - slab->s_mem;
> > > > > +	unsigned int index = offset / cache->buffer_size;
> > > > > +
> > > > > +	return index_to_obj(cache, slab, index);
> > > > > +}
> > > >
> > > > A minor nit: I think this would be more readable if you separated
> > > > variable declarations from the initializations. Also, you can probably
> > > > drop the inline from the function declaration and let GCC decide what to
> > > > do.
> > >
> > > Thats debatable. I find the setting up a number of variables that
> > > are all dependend in the above manner very readable. They are
> > > usually repetitive. Multiple functions use similar
> > > initializations.
> >
> > I agree with Pekka, it's clearly more readable when separated out
> > nicely:
> >
> > 	struct kmem_cache *cache;
> > 	unsigned int offset;
> > 	unsigned int index;
> > 	struct page *page;
> > 	struct slab *slab;
> >
> > 	page	= virt_to_head_page(portion);
> > 	slab	= page_get_slab(page);
> > 	cache	= page_get_cache(page);
> >
> > 	offset	= portion - slab->s_mem;
> > 	index	= offset / cache->buffer_size;
> >
> > The original form is hard to read due to lack of structure.
> 
> Structure can also be established differently:
> 
> static inline void *portion_to_obj(void *portion)
> {
> 	struct page *page = virt_to_head_page(portion);
> 	struct slab *slab = page_get_slab(page);
> 	struct kmem_cache *cache = page_get_cache(page);
> 
> 	unsigned int offset = portion - slab->s_mem;
> 	unsigned int index = offset / cache->buffer_size;
> 
> 	return index_to_obj(cache, slab, index);

It's still not as readable to me as the version i posted above and 
confusing as well, due to the newline in the middle of local 
variable definitions.

> It would be good if the whole series of actions that need to be 
> taken in order for the function to "get to know" the slab the 
> object parms would be simpler. Like its done in ruby
> 
> 	(page, slab, cache) = get_slab_info(portion)
> 
> 	(offset, index) = get_position_info(slab, portion)
> 
> But how can this be done in C without weird pointer passing?

The version i posted is pretty compact visually. The actual type 
enumeration is repetitive and it's often a meaningless pattern.

What matters is this sequence of symbols:

> > 	page	= virt_to_head_page(portion);
> > 	slab	= page_get_slab(page);
> > 	cache	= page_get_cache(page);
> >
> > 	offset	= portion - slab->s_mem;
> > 	index	= offset / cache->buffer_size;

... and anyone versed in slab internals will know the type of these 
variables without having to look them up. (using variable names 
consistently through a full subsystem is important for this reason)
 
Pairing them up with their base types just obscures the real logic.

That is one reason why i generally use the 'reverse christmas tree' 
type of local variable definition blocks:

> > 	struct kmem_cache *cache;
> > 	unsigned int offset;
> > 	unsigned int index;
> > 	struct page *page;
> > 	struct slab *slab;

As the trained eye will just want to skip over this as irrelevant 
fluff and the shape makes this the easiest (the less complex a shape 
is geometrically, the less 'eye skipping overhead' there is).

Anyway, these are nuances and if you go strictly by what's minimally 
required by Documentation/CodingStyle you can stop a lot sooner than 
having to bother about such fine details. The original version was 
certainly acceptable - it's just that IMO Pekka was right that it 
can be done better.

	Ingo

Re: [PATCH -mm 1/6] slab: introduce __kfree_rcu

[Pekka Enberg]

On Mon, Mar 23, 2009 at 6:56 PM, Ingo Molnar <mingo@elte.hu> wrote:
> Anyway, these are nuances and if you go strictly by what's minimally
> required by Documentation/CodingStyle you can stop a lot sooner than
> having to bother about such fine details. The original version was
> certainly acceptable - it's just that IMO Pekka was right that it
> can be done better.

Well, like I said, it was a minor nitpick, that's all. I would prefer
it the way I and Ingo suggested but if you don't want to change it,
that's fine as well.

Re: [PATCH -mm 1/6] slab: introduce __kfree_rcu

[Christoph Lameter]

On Mon, 23 Mar 2009, Pekka Enberg wrote:

> Well, like I said, it was a minor nitpick, that's all. I would prefer
> it the way I and Ingo suggested but if you don't want to change it,
> that's fine as well.

Maybe put some macros in to make these "pickup the context" thingies more
readable. Or would a macro obfuscate things?

#define GET_SLAB_CONTEXT(pointer, slab, cache, page)

	and

#define GET_OBJECT_CONTEXT(pointer, slab, index, offset) ?

Re: [PATCH -mm 1/6] slab: introduce __kfree_rcu

[Pekka Enberg]

On Mon, 23 Mar 2009, Pekka Enberg wrote:
>> Well, like I said, it was a minor nitpick, that's all. I would prefer
>> it the way I and Ingo suggested but if you don't want to change it,
>> that's fine as well.

Christoph Lameter wrote:
> Maybe put some macros in to make these "pickup the context" thingies more
> readable. Or would a macro obfuscate things?
> 
> #define GET_SLAB_CONTEXT(pointer, slab, cache, page)
> 
> 	and
> 
> #define GET_OBJECT_CONTEXT(pointer, slab, index, offset) ?

It would obfuscate things :-)

Re: [PATCH -mm 1/6] slab: introduce __kfree_rcu

[Christoph Lameter]

On Mon, 23 Mar 2009, Pekka Enberg wrote:

> > #define GET_SLAB_CONTEXT(pointer, slab, cache, page)
> >
> > 	and
> >
> > #define GET_OBJECT_CONTEXT(pointer, slab, index, offset) ?
>
> It would obfuscate things :-)

Guess I better start writing yet another better c compiler with Ruby
elements..... hehehe. yabcc?