lkml.org 
[lkml]   [2017]   [Dec]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v20 3/7 RESEND] xbitmap: add more operations
From
Date
Matthew Wilcox wrote:
> +/**
> + * xb_set_bit() - Set a bit in the XBitmap.
> + * @xb: The XBitmap.
> + * @bit: Index of the bit to set.
> + *
> + * This function is used to set a bit in the xbitmap.
> + *
> + * Return: 0 on success. -ENOMEM if memory could not be allocated.
> + */
> +int xb_set_bit(struct xb *xb, unsigned long bit)
> +{
> + unsigned long index = bit / IDA_BITMAP_BITS;
> + struct radix_tree_root *root = &xb->xbrt;
> + struct radix_tree_iter iter;
> + void __rcu **slot;
> + struct ida_bitmap *bitmap;
> +
> + bit %= IDA_BITMAP_BITS;
> + radix_tree_iter_init(&iter, index);
> + slot = idr_get_free_cmn(root, &iter, GFP_NOWAIT | __GFP_NOWARN, index);
> + if (IS_ERR(slot)) {
> + if (slot == ERR_PTR(-ENOSPC))
> + return 0; /* Already set */

Why already set? I guess something is there, but is it guaranteed that
there is a bitmap with the "bit" set?

> + return -ENOMEM;
> + }
> + 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));
nbit = 0;
assert(xb_find_set(&xb1, 2048, &nbit) == true);
assert(nbit == 2000);
assert(xb_find_set(&xb1, 2002, &nbit) == true);
assert(nbit == 2000);
nbit = 2002;
assert(xb_find_set(&xb1, 2041, &nbit) == true);
assert(nbit == 2040);
nbit = 2002;
assert(xb_find_set(&xb1, 2040, &nbit) == true);
assert(nbit == 2040);
nbit = 2000;
assert(xb_find_zero(&xb1, 2048, &nbit) == true);
assert(nbit == 2002);
nbit = 2048;
assert(xb_find_zero(&xb1, 2060, &nbit) == true);
assert(nbit == 2048);
xb_zero(&xb1, 0, 2047);
nbit = 0;
assert(xb_find_set(&xb1, 2048, &nbit) == false);
assert(nbit == 0);
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.

If bitmap == NULL at this_cpu_xchg(ida_bitmap, NULL) is allowed,
you can use kzalloc(sizeof(*bitmap), GFP_NOWAIT | __GFP_NOWARN)
and get rid of xb_preload()/xb_preload_end().

You are using idr_get_free_cmn(GFP_NOWAIT | __GFP_NOWARN), which
means that the caller has to be prepared for allocation failure
when calling xb_set_bit(). Thus, there is no need to use preload
in order to avoid failing to allocate "bitmap".



Also, please clarify why it is OK to just return here.
I don't know what

radix_tree_iter_replace(root, &iter, slot, bitmap);

is doing. If you created a slot but did not assign "bitmap",
what the caller of xb_test_bit() etc. will find? If there is an
assumption about this slot, won't this cause a problem?

> + memset(bitmap, 0, sizeof(*bitmap));
> + radix_tree_iter_replace(root, &iter, slot, bitmap);
> + }
> +
> + __set_bit(bit, bitmap->bitmap);
> + if (bitmap_full(bitmap->bitmap, IDA_BITMAP_BITS))
> + radix_tree_iter_tag_clear(root, &iter, IDR_FREE);
> + return 0;
> +}

\
 
 \ /
  Last update: 2017-12-23 04:00    [W:0.100 / U:20.980 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site