search for: xb_preload

...d xb_set_page(struct virtio_balloon *vb, > + struct page *page, > + unsigned long *pfn_min, > + unsigned long *pfn_max) > +{ > + unsigned long pfn = page_to_pfn(page); > + > + *pfn_min = min(pfn, *pfn_min); > + *pfn_max = max(pfn, *pfn_max); > + xb_preload(GFP_KERNEL); > + xb_set_bit(&vb->page_xb, pfn); > + xb_preload_end(); > +} > + So, this will allocate memory ... > @@ -198,9 +327,12 @@ static unsigned leak_balloon(struct virtio_balloon *vb, size_t num) > struct page *page; > struct balloon_dev_info *vb_dev_info...

...d xb_set_page(struct virtio_balloon *vb, > + struct page *page, > + unsigned long *pfn_min, > + unsigned long *pfn_max) > +{ > + unsigned long pfn = page_to_pfn(page); > + > + *pfn_min = min(pfn, *pfn_min); > + *pfn_max = max(pfn, *pfn_max); > + xb_preload(GFP_KERNEL); > + xb_set_bit(&vb->page_xb, pfn); > + xb_preload_end(); > +} > + So, this will allocate memory ... > @@ -198,9 +327,12 @@ static unsigned leak_balloon(struct virtio_balloon *vb, size_t num) > struct page *page; > struct balloon_dev_info *vb_dev_info...

...at virtballoon_oom_notify()" . >>>> Without the lock being held, why couldn't we use __GFP_DIRECT_RECLAIM at >>>> xb_set_page()? >>> Because of dependency shown below. >>> >>> leak_balloon() >>> xb_set_page() >>> xb_preload(GFP_KERNEL) >>> kmalloc(GFP_KERNEL) >>> __alloc_pages_may_oom() >>> Takes oom_lock >>> out_of_memory() >>> blocking_notifier_call_chain() >>> leak_balloon() >>>...

...nused radix tree node left in the tree. - xb_clear_bit: change it to be a void function, since the original implementation reurns nothing than a 0. - remove the comment above "#define XB_INDEX_BITS", because it causes confusion based on the feedbacks from the previous discussion; - xb_preload: with the original implementation, the CPU that successfully do __radix_tree_preload() may get into sleep by kmalloc(), which has a risk of getting the caller of xb_preload() scheduled to another CPU after waken up, and the new CPU may not have radix_tree_node pre-allocated there, this will...

...to use that API. > Assuming such thing leads to incorrect usage. Sure. Would you like to submit a patch? > > > 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(). > > > > No, we can't. GFP_NOWAIT | __GFP_NOWARN won't try very hard to allocate > > memory. There's no reason to fail the call if the user is in a context > > where they can try harder to free memory. > > But there is no reason to us...

...map = 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) --...

...nbits) + return ret + index * IDA_BITMAP_BITS; + } else { + return bit + index * IDA_BITMAP_BITS; + } + bit = 0; + index++; + } + + return size; +} +EXPORT_SYMBOL(xb_find_zero); + #ifndef __KERNEL__ static DEFINE_XB(xb1); @@ -111,6 +256,64 @@ void xbitmap_check_bit(unsigned long bit) xb_preload_end(); } +static void xbitmap_check_bit_range(void) +{ + /* Regular test1: node = NULL */ + xb_preload(GFP_KERNEL); + xb_set_bit(&xb1, 700); + xb_preload_end(); + assert(xb_find_set(&xb1, ULONG_MAX, 0) == 700); + assert(xb_find_set(&xb1, ULONG_MAX, 800) == ULONG_MAX); + xb_clear_bit_...

...nbits) + return ret + index * IDA_BITMAP_BITS; + } else { + return bit + index * IDA_BITMAP_BITS; + } + bit = 0; + index++; + } + + return size; +} +EXPORT_SYMBOL(xb_find_zero); + #ifndef __KERNEL__ static DEFINE_XB(xb1); @@ -111,6 +256,64 @@ void xbitmap_check_bit(unsigned long bit) xb_preload_end(); } +static void xbitmap_check_bit_range(void) +{ + /* Regular test1: node = NULL */ + xb_preload(GFP_KERNEL); + xb_set_bit(&xb1, 700); + xb_preload_end(); + assert(xb_find_set(&xb1, ULONG_MAX, 0) == 700); + assert(xb_find_set(&xb1, ULONG_MAX, 800) == ULONG_MAX); + xb_clear_bit_...

...om> Cc: Matthew Wilcox <mawilcox at microsoft.com> Cc: Andrew Morton <akpm at linux-foundation.org> Cc: Michal Hocko <mhocko at kernel.org> Cc: Michael S. Tsirkin <mst at redhat.com> Cc: Tetsuo Handa <penguin-kernel at I-love.SAKURA.ne.jp> v16->v17 ChangeLog: 1) xb_preload: allocate ida bitmap before __radix_tree_preload() to avoid kmalloc with preemption disabled. Also change this function to return with preemption not disabled on error. 2) xb_preload_and_set_bit: a wrapper of xb_preload and xb_set_bit, for the convenience of usage. v15->v16 ChangeLog: 1) coding...

...>> "[PATCH] virtio: avoid possible OOM lockup at virtballoon_oom_notify()" . >> Without the lock being held, why couldn't we use __GFP_DIRECT_RECLAIM at >> xb_set_page()? > Because of dependency shown below. > > leak_balloon() > xb_set_page() > xb_preload(GFP_KERNEL) > kmalloc(GFP_KERNEL) > __alloc_pages_may_oom() > Takes oom_lock > out_of_memory() > blocking_notifier_call_chain() > leak_balloon() > xb_set_page() > xb_preload...

On Tue, Dec 12, 2017 at 07:55:55PM +0800, Wei Wang wrote: > +int xb_preload_and_set_bit(struct xb *xb, unsigned long bit, gfp_t gfp); I'm struggling to understand when one would use this. The xb_ API requires you to handle your own locking. But specifying GFP flags here implies you can sleep. So ... um ... there's no locking? > +void xb_clear_bit_range(stru...

On Tue, Dec 12, 2017 at 07:55:55PM +0800, Wei Wang wrote: > +int xb_preload_and_set_bit(struct xb *xb, unsigned long bit, gfp_t gfp); I'm struggling to understand when one would use this. The xb_ API requires you to handle your own locking. But specifying GFP flags here implies you can sleep. So ... um ... there's no locking? > +void xb_clear_bit_range(stru...

...should not leak. > - Redid a lot of the kernel-doc. Thanks for adding it! I removed mentions > of implementation details, leaving only information useful to those who > are using it. > > Compared to the version I put out back in February: > - Accepted your change to xb_preload(); I think it's an improvement. > - Rewrote xb_set_bit() to use the radix_tree_iter_ family of functions. > Should improve performance for deep trees. > - Rewrote xb_clear_bit() for the same reason. > - Left out the use of radix tree exceptional entries. Thanks for taking...

...should not leak. > - Redid a lot of the kernel-doc. Thanks for adding it! I removed mentions > of implementation details, leaving only information useful to those who > are using it. > > Compared to the version I put out back in February: > - Accepted your change to xb_preload(); I think it's an improvement. > - Rewrote xb_set_bit() to use the radix_tree_iter_ family of functions. > Should improve performance for deep trees. > - Rewrote xb_clear_bit() for the same reason. > - Left out the use of radix tree exceptional entries. Thanks for taking...

...map) { >>>> + 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 >>...

...signed long max); +bool xb_find_set(const struct xb *xb, unsigned long max, unsigned long *bit); +bool xb_find_zero(const struct xb *xb, unsigned long max, unsigned long *bit); + +static inline bool xb_empty(const struct xb *xb) +{ + return radix_tree_empty(&xb->xbrt); +} + +int __must_check xb_preload(gfp_t); + +static inline void xb_preload_end(void) +{ + preempt_enable(); +} diff --git a/lib/Makefile b/lib/Makefile index d11c48e..08a8183 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -19,7 +19,7 @@ KCOV_INSTRUMENT_dynamic_debug.o := n lib-y := ctype.o string.o vsprintf.o cmdline.o \ rbt...

...caller to retry, so the memory should not leak. - Redid a lot of the kernel-doc. Thanks for adding it! I removed mentions of implementation details, leaving only information useful to those who are using it. Compared to the version I put out back in February: - Accepted your change to xb_preload(); I think it's an improvement. - Rewrote xb_set_bit() to use the radix_tree_iter_ family of functions. Should improve performance for deep trees. - Rewrote xb_clear_bit() for the same reason. - Left out the use of radix tree exceptional entries. Thanks for taking them out! Keeps it...

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. > +int xb_set_bit(struct xb *xb, unsigned long bit) > +{ > + int err;...

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. > +int xb_set_bit(struct xb *xb, unsigned long bit) > +{ > + int err;...

On 12/16/2017 07:28 PM, Tetsuo Handa wrote: > Wei Wang wrote: >> On 12/16/2017 02:42 AM, Matthew Wilcox wrote: >>> On Tue, Dec 12, 2017 at 07:55:55PM +0800, Wei Wang wrote: >>>> +int xb_preload_and_set_bit(struct xb *xb, unsigned long bit, gfp_t gfp); >>> I'm struggling to understand when one would use this. The xb_ API >>> requires you to handle your own locking. But specifying GFP flags >>> here implies you can sleep. So ... um ... there's no lockin...