> 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. > > Also remove the pvector_publish() call. The pvector is not being changed > in dpcls_subtable_lookup_reprobe(), only the data pointed to by pointers > in the vector are being changed.
Hi Cian, Thanks for the patch, in general this looks ok to me, but I realize Ilya had a few comments on the v1. I think these are addressed here but maybe Ilya would like to confirm before sign off? @Ilya Maximets should this go into branch 2.17 as well before release? Thanks Ian > > Fixes: 3d018c3ea79d ("dpif-netdev: add subtable lookup prio set command.") > Reported-by: Ilya Maximets <i.maxim...@ovn.org> > Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2022- > January/390757.html > Signed-off-by: Cian Ferriter <cian.ferri...@intel.com> > --- > lib/dpif-netdev-private-dpcls.h | 6 ++++-- > lib/dpif-netdev.c | 20 ++++++++++++++++---- > 2 files changed, 20 insertions(+), 6 deletions(-) > > diff --git a/lib/dpif-netdev-private-dpcls.h b/lib/dpif-netdev-private-dpcls.h > index 7c4a840cb..0d5da73c7 100644 > --- a/lib/dpif-netdev-private-dpcls.h > +++ b/lib/dpif-netdev-private-dpcls.h > @@ -83,8 +83,10 @@ struct dpcls_subtable { > /* The lookup function to use for this subtable. If there is a known > * 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; > + * the lookup function for this particular subtable. The lookup function > + * can be used at any time by a PMD thread, so it's declared as an atomic > + * here to prevent garbage from being read. */ > + ATOMIC(dpcls_subtable_lookup_func) lookup_func; > > /* 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..71f608a7c 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,12 @@ 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 to avoid garbage data being > read > + * by the PMD thread. > */ > - subtable->lookup_func = dpcls_subtable_get_best_impl(unit0, unit1); > + atomic_init(&subtable->lookup_func, > + dpcls_subtable_get_best_impl(unit0, unit1)); > > cmap_insert(&cls->subtables_map, &subtable->cmap_node, mask->hash); > /* Add the new subtable at the end of the pvector (with no hits yet) */ > @@ -9767,6 +9772,10 @@ dpcls_find_subtable(struct dpcls *cls, const struct > netdev_flow_key *mask) > /* Checks for the best available implementation for each subtable lookup > * function, and assigns it as the lookup function pointer for each subtable. > * Returns the number of subtables that have changed lookup implementation. > + * This function requires holding a flow_mutex when called. This is to make > + * sure modifications done by this function are not overwritten. This could > + * happen if dpcls_sort_subtable_vector() is called at the same time as this > + * function. > */ > static uint32_t > dpcls_subtable_lookup_reprobe(struct dpcls *cls) > @@ -9779,10 +9788,13 @@ 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 subtable lookup function atomically to avoid garbage data > + * being read by the PMD thread. */ > + atomic_store_relaxed(&subtable->lookup_func, > + dpcls_subtable_get_best_impl(u0_bits, u1_bits)); > subtables_changed += (old_func != subtable->lookup_func); > } > - pvector_publish(pvec); > > return subtables_changed; > } > -- > 2.25.1 > > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev