| Message ID | 20190527093842.10701-5-urezki@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Some cleanups for the KVA/vmalloc | expand |
On Mon, 27 May 2019 11:38:42 +0200 "Uladzislau Rezki (Sony)" <urezki@gmail.com> wrote: > Move the BUG_ON()/RB_EMPTY_NODE() check under unlink_va() > function, it means if an empty node gets freed it is a BUG > thus is considered as faulty behaviour. Can we switch it to a WARN_ON(). We are trying to remove all BUG_ON()s. If a user wants to crash on warning, there's a sysctl for that. But crashing the system can make it hard to debug. Especially if it is hit by someone without a serial console, and the machine just hangs in X. That is very annoying. With a WARN_ON, you at least get a chance to see the crash dump. -- Steve > > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com> > --- > mm/vmalloc.c | 24 +++++++++--------------- > 1 file changed, 9 insertions(+), 15 deletions(-) > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c > index 371aba9a4bf1..340959b81228 100644 > --- a/mm/vmalloc.c > +++ b/mm/vmalloc.c > @@ -533,20 +533,16 @@ link_va(struct vmap_area *va, struct rb_root *root, > static __always_inline void > unlink_va(struct vmap_area *va, struct rb_root *root) > { > - /* > - * During merging a VA node can be empty, therefore > - * not linked with the tree nor list. Just check it. > - */ > - if (!RB_EMPTY_NODE(&va->rb_node)) { > - if (root == &free_vmap_area_root) > - rb_erase_augmented(&va->rb_node, > - root, &free_vmap_area_rb_augment_cb); > - else > - rb_erase(&va->rb_node, root); > + BUG_ON(RB_EMPTY_NODE(&va->rb_node)); > > - list_del(&va->list); > - RB_CLEAR_NODE(&va->rb_node); > - } > + if (root == &free_vmap_area_root) > + rb_erase_augmented(&va->rb_node, > + root, &free_vmap_area_rb_augment_cb); > + else > + rb_erase(&va->rb_node, root); > + > + list_del(&va->list); > + RB_CLEAR_NODE(&va->rb_node); > } > > #if DEBUG_AUGMENT_PROPAGATE_CHECK > @@ -1187,8 +1183,6 @@ EXPORT_SYMBOL_GPL(unregister_vmap_purge_notifier); > > static void __free_vmap_area(struct vmap_area *va) > { > - BUG_ON(RB_EMPTY_NODE(&va->rb_node)); > - > /* > * Remove from the busy tree/list. > */
> > Move the BUG_ON()/RB_EMPTY_NODE() check under unlink_va() > > function, it means if an empty node gets freed it is a BUG > > thus is considered as faulty behaviour. > > Can we switch it to a WARN_ON(). We are trying to remove all BUG_ON()s. > If a user wants to crash on warning, there's a sysctl for that. But > crashing the system can make it hard to debug. Especially if it is hit > by someone without a serial console, and the machine just hangs in X. > That is very annoying. > > With a WARN_ON, you at least get a chance to see the crash dump. Yes we can. Even though it is considered as faulty behavior it is not a good reason to trigger a BUG. I will fix that. Thank you! -- Vlad Rezki
On Mon, May 27, 2019 at 11:38:42AM +0200, Uladzislau Rezki (Sony) wrote: > Move the BUG_ON()/RB_EMPTY_NODE() check under unlink_va() > function, it means if an empty node gets freed it is a BUG > thus is considered as faulty behaviour. It's not exactly clear from the description, why it's better. Also, do we really need a BUG_ON() in either place? Isn't something like this better? diff --git a/mm/vmalloc.c b/mm/vmalloc.c index c42872ed82ac..2df0e86d6aff 100644 --- a/mm/vmalloc.c +++ b/mm/vmalloc.c @@ -1118,7 +1118,8 @@ EXPORT_SYMBOL_GPL(unregister_vmap_purge_notifier); static void __free_vmap_area(struct vmap_area *va) { - BUG_ON(RB_EMPTY_NODE(&va->rb_node)); + if (WARN_ON_ONCE(RB_EMPTY_NODE(&va->rb_node))) + return; /* * Remove from the busy tree/list. Thanks!
Hello, Roman! > > Move the BUG_ON()/RB_EMPTY_NODE() check under unlink_va() > > function, it means if an empty node gets freed it is a BUG > > thus is considered as faulty behaviour. > > It's not exactly clear from the description, why it's better. > It is rather about if "unlink" happens on unhandled node it is faulty behavior. Something that clearly written in stone. We used to call "unlink" on detached node during merge, but after: [PATCH v3 3/4] mm/vmap: get rid of one single unlink_va() when merge it is not supposed to be ever happened across the logic. > > Also, do we really need a BUG_ON() in either place? > Historically we used to have the BUG_ON there. We can get rid of it for sure. But in this case, it would be harder to find a head or tail of it when the crash occurs, soon or later. > Isn't something like this better? > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c > index c42872ed82ac..2df0e86d6aff 100644 > --- a/mm/vmalloc.c > +++ b/mm/vmalloc.c > @@ -1118,7 +1118,8 @@ EXPORT_SYMBOL_GPL(unregister_vmap_purge_notifier); > > static void __free_vmap_area(struct vmap_area *va) > { > - BUG_ON(RB_EMPTY_NODE(&va->rb_node)); > + if (WARN_ON_ONCE(RB_EMPTY_NODE(&va->rb_node))) > + return; > I was thinking about WARN_ON_ONCE. The concern was about if the message gets lost due to kernel ring buffer. Therefore i used that. I am not sure if we have something like WARN_ONE_RATELIMIT that would be the best i think. At least it would indicate if a warning happens periodically or not. Any thoughts? Thanks for the comments! -- Vlad Rezki
On Wed, May 29, 2019 at 03:58:17PM +0200, Uladzislau Rezki wrote: > Hello, Roman! > > > > Move the BUG_ON()/RB_EMPTY_NODE() check under unlink_va() > > > function, it means if an empty node gets freed it is a BUG > > > thus is considered as faulty behaviour. > > > > It's not exactly clear from the description, why it's better. > > > It is rather about if "unlink" happens on unhandled node it is > faulty behavior. Something that clearly written in stone. We used > to call "unlink" on detached node during merge, but after: > > [PATCH v3 3/4] mm/vmap: get rid of one single unlink_va() when merge > > it is not supposed to be ever happened across the logic. > > > > > Also, do we really need a BUG_ON() in either place? > > > Historically we used to have the BUG_ON there. We can get rid of it > for sure. But in this case, it would be harder to find a head or tail > of it when the crash occurs, soon or later. > > > Isn't something like this better? > > > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c > > index c42872ed82ac..2df0e86d6aff 100644 > > --- a/mm/vmalloc.c > > +++ b/mm/vmalloc.c > > @@ -1118,7 +1118,8 @@ EXPORT_SYMBOL_GPL(unregister_vmap_purge_notifier); > > > > static void __free_vmap_area(struct vmap_area *va) > > { > > - BUG_ON(RB_EMPTY_NODE(&va->rb_node)); > > + if (WARN_ON_ONCE(RB_EMPTY_NODE(&va->rb_node))) > > + return; > > > I was thinking about WARN_ON_ONCE. The concern was about if the > message gets lost due to kernel ring buffer. Therefore i used that. > I am not sure if we have something like WARN_ONE_RATELIMIT that > would be the best i think. At least it would indicate if a warning > happens periodically or not. > > Any thoughts? Hello, Uladzislau! I don't have a strong opinion here. If you're worried about losing the message, WARN_ON() should be fine here. I don't think that this event will happen often, if at all. Thanks!
Hello, Roman! On Wed, May 29, 2019 at 04:26:43PM +0000, Roman Gushchin wrote: > On Wed, May 29, 2019 at 03:58:17PM +0200, Uladzislau Rezki wrote: > > Hello, Roman! > > > > > > Move the BUG_ON()/RB_EMPTY_NODE() check under unlink_va() > > > > function, it means if an empty node gets freed it is a BUG > > > > thus is considered as faulty behaviour. > > > > > > It's not exactly clear from the description, why it's better. > > > > > It is rather about if "unlink" happens on unhandled node it is > > faulty behavior. Something that clearly written in stone. We used > > to call "unlink" on detached node during merge, but after: > > > > [PATCH v3 3/4] mm/vmap: get rid of one single unlink_va() when merge > > > > it is not supposed to be ever happened across the logic. > > > > > > > > Also, do we really need a BUG_ON() in either place? > > > > > Historically we used to have the BUG_ON there. We can get rid of it > > for sure. But in this case, it would be harder to find a head or tail > > of it when the crash occurs, soon or later. > > > > > Isn't something like this better? > > > > > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c > > > index c42872ed82ac..2df0e86d6aff 100644 > > > --- a/mm/vmalloc.c > > > +++ b/mm/vmalloc.c > > > @@ -1118,7 +1118,8 @@ EXPORT_SYMBOL_GPL(unregister_vmap_purge_notifier); > > > > > > static void __free_vmap_area(struct vmap_area *va) > > > { > > > - BUG_ON(RB_EMPTY_NODE(&va->rb_node)); > > > + if (WARN_ON_ONCE(RB_EMPTY_NODE(&va->rb_node))) > > > + return; > > > > > I was thinking about WARN_ON_ONCE. The concern was about if the > > message gets lost due to kernel ring buffer. Therefore i used that. > > I am not sure if we have something like WARN_ONE_RATELIMIT that > > would be the best i think. At least it would indicate if a warning > > happens periodically or not. > > > > Any thoughts? > > Hello, Uladzislau! > > I don't have a strong opinion here. If you're worried about losing the message, > WARN_ON() should be fine here. I don't think that this event will happen often, > if at all. > If it happens then we are in trouble :) I prefer to keep it here as of now, later on will see. Anyway, let's keep it and i will update it with: <snip> if (WARN_ON(RB_EMPTY_NODE(&va->rb_node))) return; <snip> Thank you for the comments! -- Vlad Rezki
On Mon, Jun 03, 2019 at 07:35:28PM +0200, Uladzislau Rezki wrote: > Hello, Roman! > > On Wed, May 29, 2019 at 04:26:43PM +0000, Roman Gushchin wrote: > > On Wed, May 29, 2019 at 03:58:17PM +0200, Uladzislau Rezki wrote: > > > Hello, Roman! > > > > > > > > Move the BUG_ON()/RB_EMPTY_NODE() check under unlink_va() > > > > > function, it means if an empty node gets freed it is a BUG > > > > > thus is considered as faulty behaviour. > > > > > > > > It's not exactly clear from the description, why it's better. > > > > > > > It is rather about if "unlink" happens on unhandled node it is > > > faulty behavior. Something that clearly written in stone. We used > > > to call "unlink" on detached node during merge, but after: > > > > > > [PATCH v3 3/4] mm/vmap: get rid of one single unlink_va() when merge > > > > > > it is not supposed to be ever happened across the logic. > > > > > > > > > > > Also, do we really need a BUG_ON() in either place? > > > > > > > Historically we used to have the BUG_ON there. We can get rid of it > > > for sure. But in this case, it would be harder to find a head or tail > > > of it when the crash occurs, soon or later. > > > > > > > Isn't something like this better? > > > > > > > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c > > > > index c42872ed82ac..2df0e86d6aff 100644 > > > > --- a/mm/vmalloc.c > > > > +++ b/mm/vmalloc.c > > > > @@ -1118,7 +1118,8 @@ EXPORT_SYMBOL_GPL(unregister_vmap_purge_notifier); > > > > > > > > static void __free_vmap_area(struct vmap_area *va) > > > > { > > > > - BUG_ON(RB_EMPTY_NODE(&va->rb_node)); > > > > + if (WARN_ON_ONCE(RB_EMPTY_NODE(&va->rb_node))) > > > > + return; > > > > > > > I was thinking about WARN_ON_ONCE. The concern was about if the > > > message gets lost due to kernel ring buffer. Therefore i used that. > > > I am not sure if we have something like WARN_ONE_RATELIMIT that > > > would be the best i think. At least it would indicate if a warning > > > happens periodically or not. > > > > > > Any thoughts? > > > > Hello, Uladzislau! > > > > I don't have a strong opinion here. If you're worried about losing the message, > > WARN_ON() should be fine here. I don't think that this event will happen often, > > if at all. > > > > > If it happens then we are in trouble :) I prefer to keep it here as of now, > later on will see. Anyway, let's keep it and i will update it with: > > <snip> > if (WARN_ON(RB_EMPTY_NODE(&va->rb_node))) > return; > <snip> Works for me. Thank you! > > Thank you for the comments! You're welcome! Roman
diff --git a/mm/vmalloc.c b/mm/vmalloc.c index 371aba9a4bf1..340959b81228 100644 --- a/mm/vmalloc.c +++ b/mm/vmalloc.c @@ -533,20 +533,16 @@ link_va(struct vmap_area *va, struct rb_root *root, static __always_inline void unlink_va(struct vmap_area *va, struct rb_root *root) { - /* - * During merging a VA node can be empty, therefore - * not linked with the tree nor list. Just check it. - */ - if (!RB_EMPTY_NODE(&va->rb_node)) { - if (root == &free_vmap_area_root) - rb_erase_augmented(&va->rb_node, - root, &free_vmap_area_rb_augment_cb); - else - rb_erase(&va->rb_node, root); + BUG_ON(RB_EMPTY_NODE(&va->rb_node)); - list_del(&va->list); - RB_CLEAR_NODE(&va->rb_node); - } + if (root == &free_vmap_area_root) + rb_erase_augmented(&va->rb_node, + root, &free_vmap_area_rb_augment_cb); + else + rb_erase(&va->rb_node, root); + + list_del(&va->list); + RB_CLEAR_NODE(&va->rb_node); } #if DEBUG_AUGMENT_PROPAGATE_CHECK @@ -1187,8 +1183,6 @@ EXPORT_SYMBOL_GPL(unregister_vmap_purge_notifier); static void __free_vmap_area(struct vmap_area *va) { - BUG_ON(RB_EMPTY_NODE(&va->rb_node)); - /* * Remove from the busy tree/list. */
Move the BUG_ON()/RB_EMPTY_NODE() check under unlink_va() function, it means if an empty node gets freed it is a BUG thus is considered as faulty behaviour. Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com> --- mm/vmalloc.c | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-)