| Message ID | 20210712150710.19197-3-elibr@nvidia.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | dpif-netdev offload transitions | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/github-robot | success | github build: passed |
| ovsrobot/apply-robot | success | apply and check: success |
On Mon, Jul 12, 2021 at 5:07 PM Eli Britstein <elibr@nvidia.com> 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.") Afaiu, the race was present since the introduction of the thread handling offload requests. But this Fixes: line is ok as the race was probably not a problem before actually doing full offload. Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2021-March/381296.html > Signed-off-by: Eli Britstein <elibr@nvidia.com> > Reviewed-by: Gaetan Rivet <gaetanr@nvidia.com> Copying Maxime. Patch lgtm and it fixes the issue I had seen. Reviewed-by: David Marchand <david.marchand@redhat.com>
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 <elibr@nvidia.com> > Reviewed-by: Gaetan Rivet <gaetanr@nvidia.com> > --- > 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.
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; offload = dp_netdev_alloc_flow_offload(pmd, flow, op); offload->match = *match; offload->actions = xmalloc(actions_len); @@ -3323,7 +3322,7 @@ dp_netdev_flow_add(struct dp_netdev_pmd_thread *pmd, dp_netdev_flow_hash(&flow->ufid)); queue_netdev_flow_put(pmd, flow, match, actions, actions_len, - orig_in_port); + orig_in_port, NULL); if (OVS_UNLIKELY(!VLOG_DROP_DBG((&upcall_rl)))) { struct ds ds = DS_EMPTY_INITIALIZER; @@ -3410,7 +3409,8 @@ flow_put_on_pmd(struct dp_netdev_pmd_thread *pmd, ovsrcu_set(&netdev_flow->actions, new_actions); queue_netdev_flow_put(pmd, netdev_flow, match, - put->actions, put->actions_len, ODPP_NONE); + put->actions, put->actions_len, ODPP_NONE, + old_actions); if (stats) { get_dpif_flow_status(pmd->dp, netdev_flow, stats, NULL);
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 <elibr@nvidia.com> Reviewed-by: Gaetan Rivet <gaetanr@nvidia.com> --- lib/dpif-netdev.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)