Re: [PATCH v20 3/7 RESEND] xbitmap: add more operations

[Wei Wang]

On 12/23/2017 10:33 PM, Tetsuo Handa wrote:
> > > > +	bitmap = rcu_dereference_raw(*slot);
> > > > +	if (!bitmap) {
> > > > +		bitmap = this_cpu_xchg(ida_bitmap, NULL);
> > > > +		if (!bitmap)
> > > > +			return -ENOMEM;
> > > I can't understand this. I can understand if it were
> > >
> > >    BUG_ON(!bitmap);
> > >
> > > because you called xb_preload().
> > >
> > > But
> > >
> > > 	/*
> > > 	 * Regular test 2
> > > 	 * set bit 2000, 2001, 2040
> > > 	 * Next 1 in [0, 2048)		--> 2000
> > > 	 * Next 1 in [2000, 2002)	--> 2000
> > > 	 * Next 1 in [2002, 2041)	--> 2040
> > > 	 * Next 1 in [2002, 2040)	--> none
> > > 	 * Next 0 in [2000, 2048)	--> 2002
> > > 	 * Next 0 in [2048, 2060)	--> 2048
> > > 	 */
> > > 	xb_preload(GFP_KERNEL);
> > > 	assert(!xb_set_bit(&xb1, 2000));
> > > 	assert(!xb_set_bit(&xb1, 2001));
> > > 	assert(!xb_set_bit(&xb1, 2040));
> > [...]
> > > 	xb_preload_end();
> > >
> > > you are not calling xb_preload() prior to each xb_set_bit() call.
> > > This means that, if each xb_set_bit() is not surrounded with
> > > xb_preload()/xb_preload_end(), there is possibility of hitting
> > > this_cpu_xchg(ida_bitmap, NULL) == NULL.
> > This is just a lazy test.  We "know" that the bits in the range 1024-2047
> > will all land in the same bitmap, so there's no need to preload for each
> > of them.
> Testcases also serves as how to use that API.
> Assuming such thing leads to incorrect usage.

If callers are aware that the bits that they going to record locate in 
the same bitmap, I think they should also perform the xb_ APIs with only 
one preload. So the test cases here have shown them a correct example. 
We can probably add some comments above to explain this.


Best,
Wei