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?
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
