Re: [Qemu-devel] [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