|
From: | Wei Wang |
Subject: | Re: [Qemu-devel] [PATCH v17 1/6] lib/xbitmap: Introduce xbitmap |
Date: | Mon, 06 Nov 2017 16:15:29 +0800 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 |
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
[Prev in Thread] | Current Thread | [Next in Thread] |