| Message ID | 20240209161718.1149494-1-i.maximets@ovn.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Horman |
| Headers | show |
| Series | [ovs-dev] bond: Update of recirculation rules requires write-lock. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | fail | test: fail |
On 2/9/24 17:17, Ilya Maximets wrote: > For some reason annotation is made for a read-lock, while all the > callers are correctly holding a write-lock. > > Fixes: 05df16238d43 ("ofproto/bond: Fix bond post recirc rule leak.") > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > --- > ofproto/bond.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/ofproto/bond.c b/ofproto/bond.c > index cfdf44f85..c4b3a4a45 100644 > --- a/ofproto/bond.c > +++ b/ofproto/bond.c > @@ -423,7 +423,7 @@ update_recirc_rules__(struct bond *bond) > > static void > update_recirc_rules(struct bond *bond) > - OVS_REQ_RDLOCK(rwlock) > + OVS_REQ_WRLOCK(rwlock) > { > update_recirc_rules__(bond); > } Looks good to me. Acked-by: Adrian Moreno <amorenoz@redhat.com> Thanks.
On 2/13/24 09:32, Adrian Moreno wrote: > > > On 2/9/24 17:17, Ilya Maximets wrote: >> For some reason annotation is made for a read-lock, while all the >> callers are correctly holding a write-lock. >> >> Fixes: 05df16238d43 ("ofproto/bond: Fix bond post recirc rule leak.") >> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >> --- >> ofproto/bond.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/ofproto/bond.c b/ofproto/bond.c >> index cfdf44f85..c4b3a4a45 100644 >> --- a/ofproto/bond.c >> +++ b/ofproto/bond.c >> @@ -423,7 +423,7 @@ update_recirc_rules__(struct bond *bond) >> static void >> update_recirc_rules(struct bond *bond) >> - OVS_REQ_RDLOCK(rwlock) >> + OVS_REQ_WRLOCK(rwlock) >> { >> update_recirc_rules__(bond); >> } > > Looks good to me. > > Acked-by: Adrian Moreno <amorenoz@redhat.com> Sorry, thought of something after sending the ack. If, as we're discussing in [1], we lock the global mutex in bond_unref(), there should not be any reason to keep the lock-less version of this function (update_recirc_rules__()) so we could fold both functions together. Unless you see a reason not to backport [1] to the same degree as this patch, I could merge this patch with my v2 of [1]. WDYT? [1] https://patchwork.ozlabs.org/project/openvswitch/patch/20240209070642.2412417-1-amorenoz@redhat.com/
On 2/13/24 09:37, Adrian Moreno wrote: > > > On 2/13/24 09:32, Adrian Moreno wrote: >> >> >> On 2/9/24 17:17, Ilya Maximets wrote: >>> For some reason annotation is made for a read-lock, while all the >>> callers are correctly holding a write-lock. >>> >>> Fixes: 05df16238d43 ("ofproto/bond: Fix bond post recirc rule leak.") >>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >>> --- >>> ofproto/bond.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/ofproto/bond.c b/ofproto/bond.c >>> index cfdf44f85..c4b3a4a45 100644 >>> --- a/ofproto/bond.c >>> +++ b/ofproto/bond.c >>> @@ -423,7 +423,7 @@ update_recirc_rules__(struct bond *bond) >>> static void >>> update_recirc_rules(struct bond *bond) >>> - OVS_REQ_RDLOCK(rwlock) >>> + OVS_REQ_WRLOCK(rwlock) >>> { >>> update_recirc_rules__(bond); >>> } >> >> Looks good to me. >> >> Acked-by: Adrian Moreno <amorenoz@redhat.com> > > Sorry, thought of something after sending the ack. > > If, as we're discussing in [1], we lock the global mutex in bond_unref(), there > should not be any reason to keep the lock-less version of this function > (update_recirc_rules__()) so we could fold both functions together. > > Unless you see a reason not to backport [1] to the same degree as this patch, I > could merge this patch with my v2 of [1]. > > WDYT? Yeah, sure. If you can just incorporate that change into your patch and get rid of the lock-less function entirely, that will be better. Thanks! Best regards, Ilya Maximets. > > [1] > https://patchwork.ozlabs.org/project/openvswitch/patch/20240209070642.2412417-1-amorenoz@redhat.com/ >
On Tue, Feb 13, 2024 at 11:08:54AM +0100, Ilya Maximets wrote: > On 2/13/24 09:37, Adrian Moreno wrote: > > > > > > On 2/13/24 09:32, Adrian Moreno wrote: > >> > >> > >> On 2/9/24 17:17, Ilya Maximets wrote: > >>> For some reason annotation is made for a read-lock, while all the > >>> callers are correctly holding a write-lock. > >>> > >>> Fixes: 05df16238d43 ("ofproto/bond: Fix bond post recirc rule leak.") > >>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > >>> --- > >>> ofproto/bond.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/ofproto/bond.c b/ofproto/bond.c > >>> index cfdf44f85..c4b3a4a45 100644 > >>> --- a/ofproto/bond.c > >>> +++ b/ofproto/bond.c > >>> @@ -423,7 +423,7 @@ update_recirc_rules__(struct bond *bond) > >>> static void > >>> update_recirc_rules(struct bond *bond) > >>> - OVS_REQ_RDLOCK(rwlock) > >>> + OVS_REQ_WRLOCK(rwlock) > >>> { > >>> update_recirc_rules__(bond); > >>> } > >> > >> Looks good to me. > >> > >> Acked-by: Adrian Moreno <amorenoz@redhat.com> > > > > Sorry, thought of something after sending the ack. > > > > If, as we're discussing in [1], we lock the global mutex in bond_unref(), there > > should not be any reason to keep the lock-less version of this function > > (update_recirc_rules__()) so we could fold both functions together. > > > > Unless you see a reason not to backport [1] to the same degree as this patch, I > > could merge this patch with my v2 of [1]. > > > > WDYT? > > Yeah, sure. If you can just incorporate that change into your patch > and get rid of the lock-less function entirely, that will be better. My reading of the above is that this change will be folded - in some form - into a subsequent patchset. So I am marking this as "Changes Requested" in patchwork.
On 2/15/24 13:42, Simon Horman wrote: > On Tue, Feb 13, 2024 at 11:08:54AM +0100, Ilya Maximets wrote: >> On 2/13/24 09:37, Adrian Moreno wrote: >>> >>> >>> On 2/13/24 09:32, Adrian Moreno wrote: >>>> >>>> >>>> On 2/9/24 17:17, Ilya Maximets wrote: >>>>> For some reason annotation is made for a read-lock, while all the >>>>> callers are correctly holding a write-lock. >>>>> >>>>> Fixes: 05df16238d43 ("ofproto/bond: Fix bond post recirc rule leak.") >>>>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >>>>> --- >>>>> ofproto/bond.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/ofproto/bond.c b/ofproto/bond.c >>>>> index cfdf44f85..c4b3a4a45 100644 >>>>> --- a/ofproto/bond.c >>>>> +++ b/ofproto/bond.c >>>>> @@ -423,7 +423,7 @@ update_recirc_rules__(struct bond *bond) >>>>> static void >>>>> update_recirc_rules(struct bond *bond) >>>>> - OVS_REQ_RDLOCK(rwlock) >>>>> + OVS_REQ_WRLOCK(rwlock) >>>>> { >>>>> update_recirc_rules__(bond); >>>>> } >>>> >>>> Looks good to me. >>>> >>>> Acked-by: Adrian Moreno <amorenoz@redhat.com> >>> >>> Sorry, thought of something after sending the ack. >>> >>> If, as we're discussing in [1], we lock the global mutex in bond_unref(), there >>> should not be any reason to keep the lock-less version of this function >>> (update_recirc_rules__()) so we could fold both functions together. >>> >>> Unless you see a reason not to backport [1] to the same degree as this patch, I >>> could merge this patch with my v2 of [1]. >>> >>> WDYT? >> >> Yeah, sure. If you can just incorporate that change into your patch >> and get rid of the lock-less function entirely, that will be better. > > My reading of the above is that this change will be folded - in some form - > into a subsequent patchset. So I am marking this as "Changes Requested" > in patchwork. > Thanks! Yeah, Adrian will incorporate this into the new version of: https://patchwork.ozlabs.org/project/openvswitch/patch/20240209070642.2412417-1-amorenoz@redhat.com/ Best regards, Ilya Maximets.
diff --git a/ofproto/bond.c b/ofproto/bond.c index cfdf44f85..c4b3a4a45 100644 --- a/ofproto/bond.c +++ b/ofproto/bond.c @@ -423,7 +423,7 @@ update_recirc_rules__(struct bond *bond) static void update_recirc_rules(struct bond *bond) - OVS_REQ_RDLOCK(rwlock) + OVS_REQ_WRLOCK(rwlock) { update_recirc_rules__(bond); }
For some reason annotation is made for a read-lock, while all the callers are correctly holding a write-lock. Fixes: 05df16238d43 ("ofproto/bond: Fix bond post recirc rule leak.") Signed-off-by: Ilya Maximets <i.maximets@ovn.org> --- ofproto/bond.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)