> 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 <[email protected]>
> Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2022-
> January/390757.html
> Signed-off-by: Cian Ferriter <[email protected]>
> ---
> 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
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
