[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [PATCH v20 3/7 RESEND] xbitmap: add more operations
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
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]