Re: [ovs-dev] [PATCH v2] dpif-netdev-dpcls: Make subtable reprobe thread-safe.
> >> 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? > > > > Thanks, Ian. > > I'll take a look at this patch tomorrow. > > > >> > >> @Ilya Maximets should this go into branch 2.17 as well before release? > > > > I'd say, if it applies cleanly, we should try to backport it down to 2.14, > > because the issue may cause OVS crash. > > I didn't test it, but the change looks correct to me. Thanks! > > Acked-by: Ilya Maximets Thanks Ilya, applied to master and backported as far as 2.14. Thanks Ian ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] dpif-netdev-dpcls: Make subtable reprobe thread-safe.
On 2/14/22 18:35, Ilya Maximets wrote: > On 2/14/22 17:39, Stokes, Ian 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. >>> >>> 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? > > Thanks, Ian. > I'll take a look at this patch tomorrow. > >> >> @Ilya Maximets should this go into branch 2.17 as well before release? > > I'd say, if it applies cleanly, we should try to backport it down to 2.14, > because the issue may cause OVS crash. I didn't test it, but the change looks correct to me. Thanks! Acked-by: Ilya Maximets ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] dpif-netdev-dpcls: Make subtable reprobe thread-safe.
On 2/14/22 17:39, Stokes, Ian 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.
>>
>> 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?
Thanks, Ian.
I'll take a look at this patch tomorrow.
>
> @Ilya Maximets should this go into branch 2.17 as well before release?
I'd say, if it applies cleanly, we should try to backport it down to 2.14,
because the issue may cause OVS crash.
Best regards, Ilya Maximets.
>
> Thanks
> Ian
>
>>
>> Fixes: 3d018c3ea79d ("dpif-netdev: add subtable lookup prio set command.")
>> Reported-by: Ilya Maximets
>> Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2022-
>> January/390757.html
>> Signed-off-by: Cian Ferriter
>> ---
>> 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(>flow_mutex);
>> uint32_t subtbl_changes =
>> dpcls_subtable_lookup_reprobe(cls);
>> +ovs_mutex_unlock(>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(>lookup_func,
>> +dpcls_subtable_get_best_impl(unit0, unit1));
>>
>> cmap_insert(>subtables_map, >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)
>> @@
Re: [ovs-dev] [PATCH v2] dpif-netdev-dpcls: Make subtable reprobe thread-safe.
> 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
> Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2022-
> January/390757.html
> Signed-off-by: Cian Ferriter
> ---
> 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(>flow_mutex);
> uint32_t subtbl_changes = dpcls_subtable_lookup_reprobe(cls);
> +ovs_mutex_unlock(>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(>lookup_func,
> +dpcls_subtable_get_best_impl(unit0, unit1));
>
> cmap_insert(>subtables_map, >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
[ovs-dev] [PATCH v2] dpif-netdev-dpcls: Make subtable reprobe thread-safe.
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.
Fixes: 3d018c3ea79d ("dpif-netdev: add subtable lookup prio set command.")
Reported-by: Ilya Maximets
Reported-at:
https://mail.openvswitch.org/pipermail/ovs-dev/2022-January/390757.html
Signed-off-by: Cian Ferriter
---
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(>flow_mutex);
uint32_t subtbl_changes = dpcls_subtable_lookup_reprobe(cls);
+ovs_mutex_unlock(>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(>lookup_func,
+dpcls_subtable_get_best_impl(unit0, unit1));
cmap_insert(>subtables_map, >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(>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
