lkml.org 
[lkml]   [2017]   [Nov]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v17 1/6] lib/xbitmap: Introduce xbitmap
    On 11/03/2017 06:55 PM, Tetsuo Handa wrote:
    > I'm commenting without understanding the logic.
    >
    > Wei Wang wrote:
    >> +
    >> +bool xb_preload(gfp_t gfp);
    >> +
    > Want __must_check annotation, for __radix_tree_preload() is marked
    > with __must_check annotation. By error failing to check result of
    > xb_preload() will lead to preemption kept disabled unexpectedly.
    >

    I don't disagree with this, but I find its wrappers, e.g.
    radix_tree_preload() and radix_tree_maybe_preload(), don't seem to have
    __must_chek added.


    >
    >> +int xb_set_bit(struct xb *xb, unsigned long bit)
    >> +{
    >> + int err;
    >> + unsigned long index = bit / IDA_BITMAP_BITS;
    >> + struct radix_tree_root *root = &xb->xbrt;
    >> + struct radix_tree_node *node;
    >> + void **slot;
    >> + struct ida_bitmap *bitmap;
    >> + unsigned long ebit;
    >> +
    >> + bit %= IDA_BITMAP_BITS;
    >> + ebit = bit + 2;
    >> +
    >> + err = __radix_tree_create(root, index, 0, &node, &slot);
    >> + if (err)
    >> + return err;
    >> + bitmap = rcu_dereference_raw(*slot);
    >> + if (radix_tree_exception(bitmap)) {
    >> + unsigned long tmp = (unsigned long)bitmap;
    >> +
    >> + if (ebit < BITS_PER_LONG) {
    >> + tmp |= 1UL << ebit;
    >> + rcu_assign_pointer(*slot, (void *)tmp);
    >> + return 0;
    >> + }
    >> + bitmap = this_cpu_xchg(ida_bitmap, NULL);
    >> + if (!bitmap)
    > Please write locking rules, in order to explain how memory
    > allocated by __radix_tree_create() will not leak.
    >

    For the memory allocated by __radix_tree_create(), I think we could add:

    if (!bitmap) {
    __radix_tree_delete(root, node, slot);
    break;
    }


    For the locking rules, how about adding the following "Developer notes:"
    at the top of the file:

    "
    Locks are required to ensure that concurrent calls to xb_set_bit,
    xb_preload_and_set_bit, xb_test_bit, xb_clear_bit, xb_clear_bit_range,
    xb_find_next_set_bit and xb_find_next_zero_bit, for the same ida bitmap
    will not happen.
    "

    >> +bool xb_test_bit(struct xb *xb, unsigned long bit)
    >> +{
    >> + unsigned long index = bit / IDA_BITMAP_BITS;
    >> + const struct radix_tree_root *root = &xb->xbrt;
    >> + struct ida_bitmap *bitmap = radix_tree_lookup(root, index);
    >> +
    >> + bit %= IDA_BITMAP_BITS;
    >> +
    >> + if (!bitmap)
    >> + return false;
    >> + if (radix_tree_exception(bitmap)) {
    >> + bit += RADIX_TREE_EXCEPTIONAL_SHIFT;
    >> + if (bit > BITS_PER_LONG)
    > Why not bit >= BITS_PER_LONG here?

    Yes, I think it should be ">=" here. Thanks.

    Best,
    Wei

    \
     
     \ /
      Last update: 2017-11-06 09:14    [W:6.456 / U:0.572 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site