Re: [PATCH] cache last free vmap_area to avoid restarting beginning

[Nick Piggin]

On Sat, May 22, 2010 at 06:53:53PM +0900, Minchan Kim wrote:
> Hi, Nick.
> Sorry for late review. 

No problem, thanks for reviewing.

 
> On Thu, 2010-05-06 at 02:16 +1000, Nick Piggin wrote:
> > On Wed, May 05, 2010 at 01:48:48PM +0100, Steven Whitehouse wrote:
> > @@ -348,11 +354,23 @@ retry:
> >  	if (addr + size - 1 < addr)
> >  		goto overflow;
> >  
> > -	/* XXX: could have a last_hole cache */
> > -	n = vmap_area_root.rb_node;
> > -	if (n) {
> > -		struct vmap_area *first = NULL;
> > +	if (size <= cached_hole_size || addr < cached_start || !free_vmap_cache) {
> 
> Do we need !free_vmap_cache check?
> In __free_vmap_area, we already reset whole of variables when free_vmap_cache = NULL.

You're right.


> > +		cached_hole_size = 0;
> > +		cached_start = addr;
> > +		free_vmap_cache = NULL;
> > +	}
> >  
> > +	/* find starting point for our search */
> > +	if (free_vmap_cache) {
> > +		first = rb_entry(free_vmap_cache, struct vmap_area, rb_node);
> > +		addr = ALIGN(first->va_end + PAGE_SIZE, align);
> > +
> > +	} else {
> > +		n = vmap_area_root.rb_node;
> > +		if (!n)
> > +			goto found;
> > +
> > +		first = NULL;
> >  		do {
> >  			struct vmap_area *tmp;
> >  			tmp = rb_entry(n, struct vmap_area, rb_node);
> > @@ -369,26 +387,36 @@ retry:
> >  		if (!first)
> >  			goto found;
> >  
> > -		if (first->va_end < addr) {
> > +		if (first->va_start < addr) {
> 
> I can't understand your intention.
> Why do you change va_end with va_start?

Because we don't want an area which is spanning the start address. And
it makes subsequent logic simpler.

 
> > +			BUG_ON(first->va_end < addr);
> 
> And Why do you put this BUG_ON in here?
> Could you elaborate on logic?

It seems this is wrong, so I've removed it. This is the BUG that
Steven hit, but there is another bug in there that my stress tester
wasn't triggering.

> 
> >  			n = rb_next(&first->rb_node);
> > +			addr = ALIGN(first->va_end + PAGE_SIZE, align);
> >  			if (n)
> >  				first = rb_entry(n, struct vmap_area, rb_node);
> >  			else
> >  				goto found;
> >  		}
> > +		BUG_ON(first->va_start < addr);
> 
> Ditto. 

Don't want an area spanning start address, as above.


> >  	rb_erase(&va->rb_node, &vmap_area_root);
> >  	RB_CLEAR_NODE(&va->rb_node);
> >  	list_del_rcu(&va->list);
> 
> Hmm. I will send refactoring version soon. 
> If you don't mind, let's discuss in there. :)

I just reworked the initial patch a little bit and fixed a bug in it,
if we could instead do the refactoring on top of it, that would save
me having to rediff?

I'll post it shortly.

Thanks,
Nick
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/