On 7/12/21 5:07 PM, Eli Britstein wrote:
> Association of a mark to a flow is done as part of its offload handling,
> in the offloading thread. However, the PMD thread specifies whether an
> offload request is an "add" or "modify" by the association of a mark to
> the flow.
> This is exposed to a race condition. A flow might be created with
> actions that cannot be fully offloaded, for example flooding (before MAC
> learning), and later modified to have actions that can be fully
> offloaded. If the two requests are queued before the offload thread
> handling, they are both marked as "add". When the offload thread handles
> them, the first request is partially offloaded, and the second one is
> ignored as the flow is already considered as offloaded.
>
> Fix it by specifying add/modify of an offload request by the actual flow
> state change, without relying on the mark.
>
> Fixes: 3c7330ebf036 ("netdev-offload-dpdk: Support offload of output action.")
> Signed-off-by: Eli Britstein <[email protected]>
> Reviewed-by: Gaetan Rivet <[email protected]>
> ---
> lib/dpif-netdev.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 21b0e025d..9b2b8d6d9 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -2451,7 +2451,8 @@ static void
> queue_netdev_flow_put(struct dp_netdev_pmd_thread *pmd,
> struct dp_netdev_flow *flow, struct match *match,
> const struct nlattr *actions, size_t actions_len,
> - odp_port_t orig_in_port)
> + odp_port_t orig_in_port,
> + const struct dp_netdev_actions *old_actions)
> {
> struct dp_flow_offload_item *offload;
> int op;
> @@ -2467,11 +2468,9 @@ queue_netdev_flow_put(struct dp_netdev_pmd_thread *pmd,
> ovsthread_once_done(&offload_thread_once);
> }
>
> - if (flow->mark != INVALID_FLOW_MARK) {
> - op = DP_NETDEV_FLOW_OFFLOAD_OP_MOD;
> - } else {
> - op = DP_NETDEV_FLOW_OFFLOAD_OP_ADD;
> - }
> + op = old_actions
> + ? DP_NETDEV_FLOW_OFFLOAD_OP_MOD
> + : DP_NETDEV_FLOW_OFFLOAD_OP_ADD;
The change looks good to me in general, but I think it's better to
just directly pass DP_NETDEV_FLOW_OFFLOAD_OP_MOD/ADD to the function
instead of passing 'old_actions' pointer that is not really used here.
I see that you will use it in the next patch of the set. I'll reply
to patch #3.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
