On 01/09/2018 10:42 PM, Tetsuo Handa wrote:> Wei Wang wrote:
>> - enable OOM to free inflated pages maintained in the local temporary
>> list.
> I do want to see it before applying this patch.
Fine, then what do you think of the method I shared in your post here:
https://patchwork.kernel.org/patch/10140731/
Michael, could we merge patch 3-5 first?
>
> Please carefully check how the xbitmap implementation works, and you will
> find that you are adding a lot of redundant operations with a bug.
This version mainly added some test cases, and it passes the test run
without any issue. Appreciate it if your comments could be more
specific, that would make the discussion more effective, for example, I
deliberately added "xb_find_set(xb1, 2, ULONG_MAX - 3)" for the
overflow
test, not sure if this is the "bug" you referred to, but I'm glad
to
hear your different thought.
I agree that some tests may be repeated in some degree, since we test
the implementation from different aspects, for example,
xbitmap_check_bit_range() may have already performed xb_zero() while we
specifically have another xbitmap_check_zero_bits() which may test
something that has already been tested when checking bit range. But I
think testing twice is better than omission.
Also, I left the "Regualr test1: node=NULL" case though the new
implementation doesn't explicitly use "node" as before, but that
node=NULL is still a radix tree implementation internally and that case
looks special to me, so maybe not bad to cover in the test.
You are also welcome to send a patch to remove the redundant one if you
think that's an issue. Thanks.
Best,
Wei
