Re: [PATCH 1/5][RFC][CFT] percpu fixes, part 1

[Al Viro <viro@ZenIV.linux.org.uk>]

On Thu, Mar 06, 2014 at 02:20:26PM -0500, Tejun Heo wrote:

> Can you please add why this change is necessary to the description?

OK, will do...

> Also, I think it'd be better to split addition of first_free hint to a
> separate patch.

OK, but I'm not sure how much does it simplify things, actually.
 
> > +		chunk->map[++i] = off += size;
> >  }
> 
> Do we need to pass @size in the above function?  Isn't that something
> which can be easily determined?  If @size is gonna stay, we'll need to
> update the function comment too.

It's folded into the caller in the next patch.

> > @@ -483,19 +483,27 @@ static int pcpu_alloc_area(struct pcpu_chunk *chunk, int size, int align)
> >  	int oslot = pcpu_chunk_slot(chunk);
> >  	int max_contig = 0;
> >  	int i, off;
> > +	int seen_free = 0;
> 
> bool

Umm...  Matter of taste, but OK, I'll do that.

> > @@ -570,34 +584,50 @@ static int pcpu_alloc_area(struct pcpu_chunk *chunk, int size, int align)
> >  static void pcpu_free_area(struct pcpu_chunk *chunk, int freeme)
> >  {
> >  	int oslot = pcpu_chunk_slot(chunk);
> > -	int i, off;
> > -
> > -	for (i = 0, off = 0; i < chunk->map_used; off += abs(chunk->map[i++]))
> > -		if (off == freeme)
> > -			break;
> > +	int off = 0;
> > +	unsigned i, j;
> > +	int to_free = 0;
> > +	int *p;
> > +
> > +	freeme |= 1;
> > +
> > +	i = 0;
> > +	j = chunk->map_used;
> > +	while (i != j) {
> > +		unsigned k = (i + j) / 2;
> > +		off = chunk->map[k];
> > +		if (off < freeme)
> > +			i = k + 1;
> > +		else if (off > freeme)
> > +			j = k;
> > +		else
> > +			i = j = k;
> > +	}
> >  	BUG_ON(off != freeme);
> > -	BUG_ON(chunk->map[i] > 0);
> 
> A comment explaining why ignoring the free bit during bin search is
> okay would be nice?

Huh?  We are not ignoring it - we are searching for exact value, including
the lower bit being set.  It might be worth adding a comment next to
"freeme |= 1;" before the loop, but that's it.  These two BUG_ON() fold
nicely - that's one of the reasons why I prefer to keep the offset of
area and is_free flag of the same area in one array element.  That's why
I prefer to have the first element of array to be <0,false> or <0,true>,
and add <total_size, true> as the sentry in the end.  Sure, we could
keep <offset of the next, is this one free> together instead, and make
that array one element shorter, but that way we get more complex logics,
including that search in freeing...

> > +	if (unlikely(align < 2))
> > +		align = 2;
> 
> Please add a comment explaining why the above min alignment is
> necessary.

Umm...  Will "we want the lowest bit of offset available for free/in_use
indicator" do?