| Message ID | 20230731210211.137353-3-richard.henderson@linaro.org |
|---|---|
| State | Accepted |
| Commit | 4c8baa02d36379507afd17bdea87aabe0aa32ed3 |
| Headers | show |
| Series | [PULL,01/10] util/interval-tree: Use qatomic_read for left/right while searching | expand |
01.08.2023 00:02, Richard Henderson wrote: > Ensure that the stores to rb_left and rb_right are complete before > inserting the new node into the tree. Otherwise a concurrent reader > could see garbage in the new leaf. > > Cc: qemu-stable@nongnu.org > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > util/interval-tree.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/util/interval-tree.c b/util/interval-tree.c > index 5a0ad21b2d..759562db7d 100644 > --- a/util/interval-tree.c > +++ b/util/interval-tree.c > @@ -128,7 +128,11 @@ static inline void rb_link_node(RBNode *node, RBNode *parent, RBNode **rb_link) > node->rb_parent_color = (uintptr_t)parent; > node->rb_left = node->rb_right = NULL; > > - qatomic_set(rb_link, node); > + /* > + * Ensure that node is initialized before insertion, > + * as viewed by a concurrent search. > + */ > + qatomic_set_mb(rb_link, node); FWIW, there's no qatomic_set_mb() in 8.0 and before, so this can not be directly applied to stable-8.0. This commit is missing in 8.0 before qatomic_set_mb() can be used: commit 06831001ac8949b0801e0d20c347d97339769a20 Author: Paolo Bonzini <pbonzini@redhat.com> Date: Fri Mar 3 14:37:51 2023 +0100 atomics: eliminate mb_read/mb_set I don't think it's a good idea to back-port this commit to stable-8.0. How do you think we can solve this for 8.0? Thanks, /mjt
On 8/4/23 02:02, Michael Tokarev wrote: > 01.08.2023 00:02, Richard Henderson wrote: >> Ensure that the stores to rb_left and rb_right are complete before >> inserting the new node into the tree. Otherwise a concurrent reader >> could see garbage in the new leaf. >> >> Cc: qemu-stable@nongnu.org >> Reviewed-by: Peter Maydell <peter.maydell@linaro.org> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> util/interval-tree.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/util/interval-tree.c b/util/interval-tree.c >> index 5a0ad21b2d..759562db7d 100644 >> --- a/util/interval-tree.c >> +++ b/util/interval-tree.c >> @@ -128,7 +128,11 @@ static inline void rb_link_node(RBNode *node, RBNode *parent, >> RBNode **rb_link) >> node->rb_parent_color = (uintptr_t)parent; >> node->rb_left = node->rb_right = NULL; >> - qatomic_set(rb_link, node); >> + /* >> + * Ensure that node is initialized before insertion, >> + * as viewed by a concurrent search. >> + */ >> + qatomic_set_mb(rb_link, node); > > FWIW, there's no qatomic_set_mb() in 8.0 and before, so this can not be > directly applied to stable-8.0. This commit is missing in 8.0 before > qatomic_set_mb() can be used: > > commit 06831001ac8949b0801e0d20c347d97339769a20 > Author: Paolo Bonzini <pbonzini@redhat.com> > Date: Fri Mar 3 14:37:51 2023 +0100 > > atomics: eliminate mb_read/mb_set > > I don't think it's a good idea to back-port this commit to stable-8.0. > > How do you think we can solve this for 8.0? The function is called qatomic_mb_set instead of qatomic_set_mb in stable-8.0. r~
04.08.2023 16:41, Richard Henderson wrote: > On 8/4/23 02:02, Michael Tokarev wrote: >> 01.08.2023 00:02, Richard Henderson wrote: .. >>> + qatomic_set_mb(rb_link, node); >> >> FWIW, there's no qatomic_set_mb() in 8.0 and before, so this can not be >> directly applied to stable-8.0. This commit is missing in 8.0 before >> qatomic_set_mb() can be used: >> >> commit 06831001ac8949b0801e0d20c347d97339769a20 >> Author: Paolo Bonzini <pbonzini@redhat.com> >> Date: Fri Mar 3 14:37:51 2023 +0100 >> >> atomics: eliminate mb_read/mb_set >> >> I don't think it's a good idea to back-port this commit to stable-8.0. >> >> How do you think we can solve this for 8.0? > > The function is called qatomic_mb_set instead of qatomic_set_mb in stable-8.0. Sure, qatomic_mb_set has been renamed to qatomic_set_mb by the commit I quoted above. It is just a bit awkward to rename the user like this when back-porting, especially since the commit subject mentions the new name already. It was my first thought to use the old name, but I thought I'd ask first. Ok, let's rename the function call in this patch, but keep everything else (incl. the subject) intact. I've added comment about this though. Thanks! /mjt
diff --git a/util/interval-tree.c b/util/interval-tree.c index 5a0ad21b2d..759562db7d 100644 --- a/util/interval-tree.c +++ b/util/interval-tree.c @@ -128,7 +128,11 @@ static inline void rb_link_node(RBNode *node, RBNode *parent, RBNode **rb_link) node->rb_parent_color = (uintptr_t)parent; node->rb_left = node->rb_right = NULL; - qatomic_set(rb_link, node); + /* + * Ensure that node is initialized before insertion, + * as viewed by a concurrent search. + */ + qatomic_set_mb(rb_link, node); } static RBNode *rb_next(RBNode *node)