> -----Original Message-----
> From: Ilya Maximets <[email protected]>
> Sent: Friday 4 February 2022 12:11
> To: Ferriter, Cian <[email protected]>; [email protected]
> Cc: [email protected]; Van Haaren, Harry <[email protected]>
> Subject: Re: [PATCH] dpif-netdev-dpcls: Make subtable reprobe thread-safe.
>
> On 2/2/22 15:51, Cian Ferriter wrote:
> > The subtable search function can be used at any time by a PMD thread.
> > Setting the subtable search function should be done atomically to
> > prevent garbage data from being read.
> >
> > A dpcls_subtable_lookup_reprobe() call can happen at the same time that
> > DPCLS subtables are being sorted. This could lead to modifications done
> > by the reprobe() to be lost. Prevent this from happening by locking on
> > pmd->flow_mutex. After this change both the reprobe function and a
> > subtable sort will share the flow_mutex preventing modifications by
> > either one from being lost.
> >
> > Fixes: 3d018c3ea79d ("dpif-netdev: add subtable lookup prio set command.")
> > Reported-by: Ilya Maximets <[email protected]>
> > Reported-at:
> > https://mail.openvswitch.org/pipermail/ovs-dev/2022-January/390757.html
> > Signed-off-by: Cian Ferriter <[email protected]>
>
> Hi, Cian. Thanks for working on this!
> I didn't test that, but see some comments inline.
>
> Best regards, Ilya Maximets.
>
Hi Ilya, thanks for the review. The explanations around pvector_publish were
very helpful!
I sent a new version of the patch:
http://patchwork.ozlabs.org/project/openvswitch/patch/[email protected]/
> > ---
> > lib/dpif-netdev-private-dpcls.h | 2 +-
> > lib/dpif-netdev.c | 23 +++++++++++++++++++----
> > 2 files changed, 20 insertions(+), 5 deletions(-)
> >
> > diff --git a/lib/dpif-netdev-private-dpcls.h
> > b/lib/dpif-netdev-private-dpcls.h
> > index 7c4a840cb..1b4d69605 100644
> > --- a/lib/dpif-netdev-private-dpcls.h
> > +++ b/lib/dpif-netdev-private-dpcls.h
> > @@ -84,7 +84,7 @@ struct dpcls_subtable {
> > * property of the subtable (eg: only 3 bits of miniflow metadata is
> > * used for the lookup) then this can point at an optimized version of
> > * the lookup function for this particular subtable. */
> > - dpcls_subtable_lookup_func lookup_func;
> > + ATOMIC(dpcls_subtable_lookup_func) lookup_func;
>
> Maybe a comment update saying why it's atomic?
>
Good idea, it's good to document the reason.
Updated this in the v2.
> >
> > /* Caches the masks to match a packet to, reducing runtime
> > calculations. */
> > uint64_t *mf_masks;
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > index e28e0b554..9ca6d9842 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -1074,7 +1074,9 @@ dpif_netdev_subtable_lookup_set(struct unixctl_conn
> > *conn, int argc
> OVS_UNUSED,
> > if (!cls) {
> > continue;
> > }
> > + ovs_mutex_lock(&pmd->flow_mutex);
> > uint32_t subtbl_changes =
> > dpcls_subtable_lookup_reprobe(cls);
> > + ovs_mutex_unlock(&pmd->flow_mutex);
> > if (subtbl_changes) {
> > lookup_dpcls_changed++;
> > lookup_subtable_changed += subtbl_changes;
> > @@ -9736,9 +9738,14 @@ dpcls_create_subtable(struct dpcls *cls, const
> > struct netdev_flow_key *mask)
> >
> > /* Get the preferred subtable search function for this (u0,u1)
> > subtable.
> > * The function is guaranteed to always return a valid implementation,
> > and
> > - * possibly an ISA optimized, and/or specialized implementation.
> > + * possibly an ISA optimized, and/or specialized implementation.
> > Initialize
> > + * the subtable search function atomically.
>
> Again, the comment 'why?' would be helpful. OTOH, the comment in the
> structure
> itself might be enough.
>
I think you're right. The WHY is more important than me just saying WHAT we are
doing.
Hopefully the code says WHAT we are doing, and we say WHY! :)
Updated this in the v2.
> > */
> > - subtable->lookup_func = dpcls_subtable_get_best_impl(unit0, unit1);
> > + dpcls_subtable_lookup_func best_func =
> > dpcls_subtable_get_best_impl(unit0,
> > +
> > unit1);
> > + atomic_uintptr_t *subtable_func = (void *) &subtable->lookup_func;
> > + atomic_init(subtable_func, (uintptr_t) best_func);
>
> This pattern looks a bit weird, but for some reason it's used across
> avx512-related
> parts of the code. Something like this should work just fine:
>
> atomic_init(&subtable->lookup_func,
> dpcls_subtable_get_best_impl(unit0, unit1));
>
Above works perfectly. I'm using your line in v2. Feel free to add your
Co-authored-by if that makes sense/is appropriate.
I started the weird looking pattern when making the DPIF implementation
function pointer (netdev_input_func) atomic. I remember trying to simplify the
pattern, but not quite getting what I wanted.
I'll go back and check if the other atomic function pointer code can be
simplified to look like what you suggested.
> > +
>
> Extra empty line is not needed.
>
Ooops, fixed in v2.
> >
> > cmap_insert(&cls->subtables_map, &subtable->cmap_node, mask->hash);
> > /* Add the new subtable at the end of the pvector (with no hits yet) */
> > @@ -9779,10 +9786,18 @@ dpcls_subtable_lookup_reprobe(struct dpcls *cls)
> > uint32_t u0_bits = subtable->mf_bits_set_unit0;
> > uint32_t u1_bits = subtable->mf_bits_set_unit1;
> > void *old_func = subtable->lookup_func;
> > - subtable->lookup_func = dpcls_subtable_get_best_impl(u0_bits,
> > u1_bits);
> > +
> > + /* Set the best impl to the subtable lookup function atomically. */
>
> This comment doesn't seem very useful.
>
Agreed, updated in the next version to explain why we are setting atomically.
> > + dpcls_subtable_lookup_func best_func =
> > + dpcls_subtable_get_best_impl(u0_bits, u1_bits);
> > + atomic_uintptr_t *subtable_func = (void *)
> > &subtable->lookup_func;> +
> atomic_store_relaxed(subtable_func, (uintptr_t) best_func);
>
> Same here.
>
> atomic_store_relaxed(&subtable->lookup_func,
> dpcls_subtable_get_best_impl(u0_bits, u1_bits));
>
Fixed this to use your suggested code in the v2.
> > +
>
> Please, add an empty line between declarations of variables and the rest of
> the code instead.
>
Will do in the v2.
> > subtables_changed += (old_func != subtable->lookup_func);
> > }
> > - pvector_publish(pvec);
> > + if (subtables_changed) {
> > + pvector_publish(pvec);
>
> This still does nothing useful. It's harmless, because we're under the
> flow_mutex now, but it does nothing, because we're still not modifying
> the vector, we're modifying the data to which pointers in the vector are
> pointing to.
>
> In the ideal world the vector and data it points to should be immutable.
> So, we'll need to clone the implementation of the vector, remove and destroy
> all the elements we would like to change, create modified versions and
> add them to the vector. After that we can publish it. In this scenario
> everything will be protected by rcu and no atomics required. But that
> sounds like a waste of time for this particular use case, so we're
> creating a workaround by using atomics and modifying the data while other
> threads are using it. But we're not changing the vector itself.
>
Thanks for the detailed explanation, this was really helpful! After reading the
above and looking at the other uses of pvector_publish, this makes more sense
to me. I'll remove the pvector_publish for v2.
> Would be nice to have a comment though, why this function requires holding
> a flow_mutex.
>
I'll add to the comment about this function in the v2.
> > + }
> >
> > return subtables_changed;
> > }
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
