| Message ID | 20210210152702.4898-12-elibr@nvidia.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Netdev vxlan-decap offload | expand |
On Wed, Feb 10, 2021 at 8:57 PM Eli Britstein <elibr@nvidia.com> wrote: > > Refactor offload rule creation as a pre-step towards tunnel matching > that depend on the netdev. > > Signed-off-by: Eli Britstein <elibr@nvidia.com> > Reviewed-by: Gaetan Rivet <gaetanr@nvidia.com> > --- > lib/netdev-offload-dpdk.c | 106 ++++++++++++++++---------------------- > 1 file changed, 45 insertions(+), 61 deletions(-) > > diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c > index 493cc9159..f6e668bff 100644 > --- a/lib/netdev-offload-dpdk.c > +++ b/lib/netdev-offload-dpdk.c > @@ -1009,30 +1009,6 @@ add_flow_mark_rss_actions(struct flow_actions *actions, > add_flow_action(actions, RTE_FLOW_ACTION_TYPE_END, NULL); > } > > -static struct rte_flow * > -netdev_offload_dpdk_mark_rss(struct flow_patterns *patterns, > - struct netdev *netdev, > - uint32_t flow_mark) > -{ > - struct flow_actions actions = { .actions = NULL, .cnt = 0 }; > - const struct rte_flow_attr flow_attr = { > - .group = 0, > - .priority = 0, > - .ingress = 1, > - .egress = 0 > - }; > - struct rte_flow_error error; > - struct rte_flow *flow; > - > - add_flow_mark_rss_actions(&actions, flow_mark, netdev); > - > - flow = netdev_offload_dpdk_flow_create(netdev, &flow_attr, patterns->items, > - &actions, &error); > - > - free_flow_actions(&actions); > - return flow; > -} > - > static void > add_count_action(struct flow_actions *actions) > { > @@ -1509,27 +1485,48 @@ parse_flow_actions(struct netdev *netdev, > return 0; > } > > -static struct rte_flow * > -netdev_offload_dpdk_actions(struct netdev *netdev, > - struct flow_patterns *patterns, > - struct nlattr *nl_actions, > - size_t actions_len) > +static struct ufid_to_rte_flow_data * > +create_netdev_offload(struct netdev *netdev, > + const ovs_u128 *ufid, > + struct flow_patterns *flow_patterns, > + struct flow_actions *flow_actions, > + bool enable_full, > + uint32_t flow_mark) > { > - const struct rte_flow_attr flow_attr = { .ingress = 1, .transfer = 1 }; > - struct flow_actions actions = { .actions = NULL, .cnt = 0 }; > + struct rte_flow_attr flow_attr = { .ingress = 1, .transfer = 1, }; > + struct flow_actions rss_actions = { .actions = NULL, .cnt = 0 }; > + struct rte_flow_item *items = flow_patterns->items; > + struct ufid_to_rte_flow_data *flow_data = NULL; > + bool actions_offloaded = true; > struct rte_flow *flow = NULL; > struct rte_flow_error error; > - int ret; > > - ret = parse_flow_actions(netdev, &actions, nl_actions, actions_len); > - if (ret) { > - goto out; > + if (enable_full) { > + flow = netdev_offload_dpdk_flow_create(netdev, &flow_attr, items, > + flow_actions, &error); > } > - flow = netdev_offload_dpdk_flow_create(netdev, &flow_attr, patterns->items, > - &actions, &error); > -out: > - free_flow_actions(&actions); > - return flow; > + > + if (!flow) { > + /* If we failed to offload the rule actions fallback to MARK+RSS > + * actions. > + */ A debug message might be useful here, when we fallback to mark/rss action ? > + actions_offloaded = false; > + flow_attr.transfer = 0; > + add_flow_mark_rss_actions(&rss_actions, flow_mark, netdev); > + flow = netdev_offload_dpdk_flow_create(netdev, &flow_attr, items, > + &rss_actions, &error); > + } > + > + if (flow) { > + flow_data = ufid_to_rte_flow_associate(ufid, netdev, flow, > + actions_offloaded); > + VLOG_DBG("%s: installed flow %p by ufid "UUID_FMT, > + netdev_get_name(netdev), flow, > + UUID_ARGS((struct uuid *) ufid)); > + } > + > + free_flow_actions(&rss_actions); This free is needed only when we fallback to mark/rss offload, not otherwise. > + return flow_data; > } > > static struct ufid_to_rte_flow_data * > @@ -1541,9 +1538,9 @@ netdev_offload_dpdk_add_flow(struct netdev *netdev, > struct offload_info *info) > { > struct flow_patterns patterns = { .items = NULL, .cnt = 0 }; > - struct ufid_to_rte_flow_data *flows_data = NULL; > - bool actions_offloaded = true; > - struct rte_flow *flow; > + struct flow_actions actions = { .actions = NULL, .cnt = 0 }; > + struct ufid_to_rte_flow_data *flow_data = NULL; > + int err; > > if (parse_flow_match(&patterns, match)) { > VLOG_DBG_RL(&rl, "%s: matches of ufid "UUID_FMT" are not supported", > @@ -1551,28 +1548,15 @@ netdev_offload_dpdk_add_flow(struct netdev *netdev, > goto out; > } > > - flow = netdev_offload_dpdk_actions(netdev, &patterns, nl_actions, > - actions_len); > - if (!flow) { > - /* If we failed to offload the rule actions fallback to MARK+RSS > - * actions. > - */ > - flow = netdev_offload_dpdk_mark_rss(&patterns, netdev, > - info->flow_mark); > - actions_offloaded = false; > - } > + err = parse_flow_actions(netdev, &actions, nl_actions, actions_len); > > - if (!flow) { > - goto out; > - } > - flows_data = ufid_to_rte_flow_associate(ufid, netdev, flow, > - actions_offloaded); > - VLOG_DBG("%s: installed flow %p by ufid "UUID_FMT, > - netdev_get_name(netdev), flow, UUID_ARGS((struct uuid *)ufid)); > + flow_data = create_netdev_offload(netdev, ufid, &patterns, &actions, !err, > + info->flow_mark); > > out: > free_flow_patterns(&patterns); > - return flows_data; > + free_flow_actions(&actions); > + return flow_data; > } > > static int > -- > 2.28.0.546.g385c171 >
On 2/24/2021 11:55 AM, Sriharsha Basavapatna wrote: > On Wed, Feb 10, 2021 at 8:57 PM Eli Britstein <elibr@nvidia.com> wrote: >> Refactor offload rule creation as a pre-step towards tunnel matching >> that depend on the netdev. >> >> Signed-off-by: Eli Britstein <elibr@nvidia.com> >> Reviewed-by: Gaetan Rivet <gaetanr@nvidia.com> >> --- >> lib/netdev-offload-dpdk.c | 106 ++++++++++++++++---------------------- >> 1 file changed, 45 insertions(+), 61 deletions(-) >> >> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c >> index 493cc9159..f6e668bff 100644 >> --- a/lib/netdev-offload-dpdk.c >> +++ b/lib/netdev-offload-dpdk.c >> @@ -1009,30 +1009,6 @@ add_flow_mark_rss_actions(struct flow_actions *actions, >> add_flow_action(actions, RTE_FLOW_ACTION_TYPE_END, NULL); >> } >> >> -static struct rte_flow * >> -netdev_offload_dpdk_mark_rss(struct flow_patterns *patterns, >> - struct netdev *netdev, >> - uint32_t flow_mark) >> -{ >> - struct flow_actions actions = { .actions = NULL, .cnt = 0 }; >> - const struct rte_flow_attr flow_attr = { >> - .group = 0, >> - .priority = 0, >> - .ingress = 1, >> - .egress = 0 >> - }; >> - struct rte_flow_error error; >> - struct rte_flow *flow; >> - >> - add_flow_mark_rss_actions(&actions, flow_mark, netdev); >> - >> - flow = netdev_offload_dpdk_flow_create(netdev, &flow_attr, patterns->items, >> - &actions, &error); >> - >> - free_flow_actions(&actions); >> - return flow; >> -} >> - >> static void >> add_count_action(struct flow_actions *actions) >> { >> @@ -1509,27 +1485,48 @@ parse_flow_actions(struct netdev *netdev, >> return 0; >> } >> >> -static struct rte_flow * >> -netdev_offload_dpdk_actions(struct netdev *netdev, >> - struct flow_patterns *patterns, >> - struct nlattr *nl_actions, >> - size_t actions_len) >> +static struct ufid_to_rte_flow_data * >> +create_netdev_offload(struct netdev *netdev, >> + const ovs_u128 *ufid, >> + struct flow_patterns *flow_patterns, >> + struct flow_actions *flow_actions, >> + bool enable_full, >> + uint32_t flow_mark) >> { >> - const struct rte_flow_attr flow_attr = { .ingress = 1, .transfer = 1 }; >> - struct flow_actions actions = { .actions = NULL, .cnt = 0 }; >> + struct rte_flow_attr flow_attr = { .ingress = 1, .transfer = 1, }; >> + struct flow_actions rss_actions = { .actions = NULL, .cnt = 0 }; >> + struct rte_flow_item *items = flow_patterns->items; >> + struct ufid_to_rte_flow_data *flow_data = NULL; >> + bool actions_offloaded = true; >> struct rte_flow *flow = NULL; >> struct rte_flow_error error; >> - int ret; >> >> - ret = parse_flow_actions(netdev, &actions, nl_actions, actions_len); >> - if (ret) { >> - goto out; >> + if (enable_full) { >> + flow = netdev_offload_dpdk_flow_create(netdev, &flow_attr, items, >> + flow_actions, &error); >> } >> - flow = netdev_offload_dpdk_flow_create(netdev, &flow_attr, patterns->items, >> - &actions, &error); >> -out: >> - free_flow_actions(&actions); >> - return flow; >> + >> + if (!flow) { >> + /* If we failed to offload the rule actions fallback to MARK+RSS >> + * actions. >> + */ > A debug message might be useful here, when we fallback to mark/rss action ? We can, but this is just a refactor commit, and this fallback is not new. Add it anyway? >> + actions_offloaded = false; >> + flow_attr.transfer = 0; >> + add_flow_mark_rss_actions(&rss_actions, flow_mark, netdev); >> + flow = netdev_offload_dpdk_flow_create(netdev, &flow_attr, items, >> + &rss_actions, &error); >> + } >> + >> + if (flow) { >> + flow_data = ufid_to_rte_flow_associate(ufid, netdev, flow, >> + actions_offloaded); >> + VLOG_DBG("%s: installed flow %p by ufid "UUID_FMT, >> + netdev_get_name(netdev), flow, >> + UUID_ARGS((struct uuid *) ufid)); >> + } >> + >> + free_flow_actions(&rss_actions); > This free is needed only when we fallback to mark/rss offload, not otherwise. OK. > >> + return flow_data; >> } >> >> static struct ufid_to_rte_flow_data * >> @@ -1541,9 +1538,9 @@ netdev_offload_dpdk_add_flow(struct netdev *netdev, >> struct offload_info *info) >> { >> struct flow_patterns patterns = { .items = NULL, .cnt = 0 }; >> - struct ufid_to_rte_flow_data *flows_data = NULL; >> - bool actions_offloaded = true; >> - struct rte_flow *flow; >> + struct flow_actions actions = { .actions = NULL, .cnt = 0 }; >> + struct ufid_to_rte_flow_data *flow_data = NULL; >> + int err; >> >> if (parse_flow_match(&patterns, match)) { >> VLOG_DBG_RL(&rl, "%s: matches of ufid "UUID_FMT" are not supported", >> @@ -1551,28 +1548,15 @@ netdev_offload_dpdk_add_flow(struct netdev *netdev, >> goto out; >> } >> >> - flow = netdev_offload_dpdk_actions(netdev, &patterns, nl_actions, >> - actions_len); >> - if (!flow) { >> - /* If we failed to offload the rule actions fallback to MARK+RSS >> - * actions. >> - */ >> - flow = netdev_offload_dpdk_mark_rss(&patterns, netdev, >> - info->flow_mark); >> - actions_offloaded = false; >> - } >> + err = parse_flow_actions(netdev, &actions, nl_actions, actions_len); >> >> - if (!flow) { >> - goto out; >> - } >> - flows_data = ufid_to_rte_flow_associate(ufid, netdev, flow, >> - actions_offloaded); >> - VLOG_DBG("%s: installed flow %p by ufid "UUID_FMT, >> - netdev_get_name(netdev), flow, UUID_ARGS((struct uuid *)ufid)); >> + flow_data = create_netdev_offload(netdev, ufid, &patterns, &actions, !err, >> + info->flow_mark); >> >> out: >> free_flow_patterns(&patterns); >> - return flows_data; >> + free_flow_actions(&actions); >> + return flow_data; >> } >> >> static int >> -- >> 2.28.0.546.g385c171 >>
On Wed, Feb 24, 2021 at 3:47 PM Eli Britstein <elibr@nvidia.com> wrote: > > > On 2/24/2021 11:55 AM, Sriharsha Basavapatna wrote: > > On Wed, Feb 10, 2021 at 8:57 PM Eli Britstein <elibr@nvidia.com> wrote: > >> Refactor offload rule creation as a pre-step towards tunnel matching > >> that depend on the netdev. > >> > >> Signed-off-by: Eli Britstein <elibr@nvidia.com> > >> Reviewed-by: Gaetan Rivet <gaetanr@nvidia.com> > >> --- > >> lib/netdev-offload-dpdk.c | 106 ++++++++++++++++---------------------- > >> 1 file changed, 45 insertions(+), 61 deletions(-) > >> > >> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c > >> index 493cc9159..f6e668bff 100644 > >> --- a/lib/netdev-offload-dpdk.c > >> +++ b/lib/netdev-offload-dpdk.c > >> @@ -1009,30 +1009,6 @@ add_flow_mark_rss_actions(struct flow_actions *actions, > >> add_flow_action(actions, RTE_FLOW_ACTION_TYPE_END, NULL); > >> } > >> > >> -static struct rte_flow * > >> -netdev_offload_dpdk_mark_rss(struct flow_patterns *patterns, > >> - struct netdev *netdev, > >> - uint32_t flow_mark) > >> -{ > >> - struct flow_actions actions = { .actions = NULL, .cnt = 0 }; > >> - const struct rte_flow_attr flow_attr = { > >> - .group = 0, > >> - .priority = 0, > >> - .ingress = 1, > >> - .egress = 0 > >> - }; > >> - struct rte_flow_error error; > >> - struct rte_flow *flow; > >> - > >> - add_flow_mark_rss_actions(&actions, flow_mark, netdev); > >> - > >> - flow = netdev_offload_dpdk_flow_create(netdev, &flow_attr, patterns->items, > >> - &actions, &error); > >> - > >> - free_flow_actions(&actions); > >> - return flow; > >> -} > >> - > >> static void > >> add_count_action(struct flow_actions *actions) > >> { > >> @@ -1509,27 +1485,48 @@ parse_flow_actions(struct netdev *netdev, > >> return 0; > >> } > >> > >> -static struct rte_flow * > >> -netdev_offload_dpdk_actions(struct netdev *netdev, > >> - struct flow_patterns *patterns, > >> - struct nlattr *nl_actions, > >> - size_t actions_len) > >> +static struct ufid_to_rte_flow_data * > >> +create_netdev_offload(struct netdev *netdev, > >> + const ovs_u128 *ufid, > >> + struct flow_patterns *flow_patterns, > >> + struct flow_actions *flow_actions, > >> + bool enable_full, > >> + uint32_t flow_mark) > >> { > >> - const struct rte_flow_attr flow_attr = { .ingress = 1, .transfer = 1 }; > >> - struct flow_actions actions = { .actions = NULL, .cnt = 0 }; > >> + struct rte_flow_attr flow_attr = { .ingress = 1, .transfer = 1, }; > >> + struct flow_actions rss_actions = { .actions = NULL, .cnt = 0 }; > >> + struct rte_flow_item *items = flow_patterns->items; > >> + struct ufid_to_rte_flow_data *flow_data = NULL; > >> + bool actions_offloaded = true; > >> struct rte_flow *flow = NULL; > >> struct rte_flow_error error; > >> - int ret; > >> > >> - ret = parse_flow_actions(netdev, &actions, nl_actions, actions_len); > >> - if (ret) { > >> - goto out; > >> + if (enable_full) { > >> + flow = netdev_offload_dpdk_flow_create(netdev, &flow_attr, items, > >> + flow_actions, &error); > >> } > >> - flow = netdev_offload_dpdk_flow_create(netdev, &flow_attr, patterns->items, > >> - &actions, &error); > >> -out: > >> - free_flow_actions(&actions); > >> - return flow; > >> + > >> + if (!flow) { > >> + /* If we failed to offload the rule actions fallback to MARK+RSS > >> + * actions. > >> + */ > > A debug message might be useful here, when we fallback to mark/rss action ? > We can, but this is just a refactor commit, and this fallback is not > new. Add it anyway? I think it'd be useful to add a debug message here and also in parse_flow_actions() to indicate that action offloading failed. > >> + actions_offloaded = false; > >> + flow_attr.transfer = 0; > >> + add_flow_mark_rss_actions(&rss_actions, flow_mark, netdev); > >> + flow = netdev_offload_dpdk_flow_create(netdev, &flow_attr, items, > >> + &rss_actions, &error); > >> + } > >> + > >> + if (flow) { > >> + flow_data = ufid_to_rte_flow_associate(ufid, netdev, flow, > >> + actions_offloaded); > >> + VLOG_DBG("%s: installed flow %p by ufid "UUID_FMT, > >> + netdev_get_name(netdev), flow, > >> + UUID_ARGS((struct uuid *) ufid)); > >> + } > >> + > >> + free_flow_actions(&rss_actions); > > This free is needed only when we fallback to mark/rss offload, not otherwise. > OK. > > > >> + return flow_data; > >> } > >> > >> static struct ufid_to_rte_flow_data * > >> @@ -1541,9 +1538,9 @@ netdev_offload_dpdk_add_flow(struct netdev *netdev, > >> struct offload_info *info) > >> { > >> struct flow_patterns patterns = { .items = NULL, .cnt = 0 }; > >> - struct ufid_to_rte_flow_data *flows_data = NULL; > >> - bool actions_offloaded = true; > >> - struct rte_flow *flow; > >> + struct flow_actions actions = { .actions = NULL, .cnt = 0 }; > >> + struct ufid_to_rte_flow_data *flow_data = NULL; > >> + int err; > >> > >> if (parse_flow_match(&patterns, match)) { > >> VLOG_DBG_RL(&rl, "%s: matches of ufid "UUID_FMT" are not supported", > >> @@ -1551,28 +1548,15 @@ netdev_offload_dpdk_add_flow(struct netdev *netdev, > >> goto out; > >> } > >> > >> - flow = netdev_offload_dpdk_actions(netdev, &patterns, nl_actions, > >> - actions_len); > >> - if (!flow) { > >> - /* If we failed to offload the rule actions fallback to MARK+RSS > >> - * actions. > >> - */ > >> - flow = netdev_offload_dpdk_mark_rss(&patterns, netdev, > >> - info->flow_mark); > >> - actions_offloaded = false; > >> - } > >> + err = parse_flow_actions(netdev, &actions, nl_actions, actions_len); > >> > >> - if (!flow) { > >> - goto out; > >> - } > >> - flows_data = ufid_to_rte_flow_associate(ufid, netdev, flow, > >> - actions_offloaded); > >> - VLOG_DBG("%s: installed flow %p by ufid "UUID_FMT, > >> - netdev_get_name(netdev), flow, UUID_ARGS((struct uuid *)ufid)); > >> + flow_data = create_netdev_offload(netdev, ufid, &patterns, &actions, !err, > >> + info->flow_mark); > >> > >> out: > >> free_flow_patterns(&patterns); > >> - return flows_data; > >> + free_flow_actions(&actions); > >> + return flow_data; > >> } > >> > >> static int > >> -- > >> 2.28.0.546.g385c171 > >>
On 2/25/2021 9:35 AM, Sriharsha Basavapatna wrote: > On Wed, Feb 24, 2021 at 3:47 PM Eli Britstein <elibr@nvidia.com> wrote: >> >> On 2/24/2021 11:55 AM, Sriharsha Basavapatna wrote: >>> On Wed, Feb 10, 2021 at 8:57 PM Eli Britstein <elibr@nvidia.com> wrote: >>>> Refactor offload rule creation as a pre-step towards tunnel matching >>>> that depend on the netdev. >>>> >>>> Signed-off-by: Eli Britstein <elibr@nvidia.com> >>>> Reviewed-by: Gaetan Rivet <gaetanr@nvidia.com> >>>> --- >>>> lib/netdev-offload-dpdk.c | 106 ++++++++++++++++---------------------- >>>> 1 file changed, 45 insertions(+), 61 deletions(-) >>>> >>>> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c >>>> index 493cc9159..f6e668bff 100644 >>>> --- a/lib/netdev-offload-dpdk.c >>>> +++ b/lib/netdev-offload-dpdk.c >>>> @@ -1009,30 +1009,6 @@ add_flow_mark_rss_actions(struct flow_actions *actions, >>>> add_flow_action(actions, RTE_FLOW_ACTION_TYPE_END, NULL); >>>> } >>>> >>>> -static struct rte_flow * >>>> -netdev_offload_dpdk_mark_rss(struct flow_patterns *patterns, >>>> - struct netdev *netdev, >>>> - uint32_t flow_mark) >>>> -{ >>>> - struct flow_actions actions = { .actions = NULL, .cnt = 0 }; >>>> - const struct rte_flow_attr flow_attr = { >>>> - .group = 0, >>>> - .priority = 0, >>>> - .ingress = 1, >>>> - .egress = 0 >>>> - }; >>>> - struct rte_flow_error error; >>>> - struct rte_flow *flow; >>>> - >>>> - add_flow_mark_rss_actions(&actions, flow_mark, netdev); >>>> - >>>> - flow = netdev_offload_dpdk_flow_create(netdev, &flow_attr, patterns->items, >>>> - &actions, &error); >>>> - >>>> - free_flow_actions(&actions); >>>> - return flow; >>>> -} >>>> - >>>> static void >>>> add_count_action(struct flow_actions *actions) >>>> { >>>> @@ -1509,27 +1485,48 @@ parse_flow_actions(struct netdev *netdev, >>>> return 0; >>>> } >>>> >>>> -static struct rte_flow * >>>> -netdev_offload_dpdk_actions(struct netdev *netdev, >>>> - struct flow_patterns *patterns, >>>> - struct nlattr *nl_actions, >>>> - size_t actions_len) >>>> +static struct ufid_to_rte_flow_data * >>>> +create_netdev_offload(struct netdev *netdev, >>>> + const ovs_u128 *ufid, >>>> + struct flow_patterns *flow_patterns, >>>> + struct flow_actions *flow_actions, >>>> + bool enable_full, >>>> + uint32_t flow_mark) >>>> { >>>> - const struct rte_flow_attr flow_attr = { .ingress = 1, .transfer = 1 }; >>>> - struct flow_actions actions = { .actions = NULL, .cnt = 0 }; >>>> + struct rte_flow_attr flow_attr = { .ingress = 1, .transfer = 1, }; >>>> + struct flow_actions rss_actions = { .actions = NULL, .cnt = 0 }; >>>> + struct rte_flow_item *items = flow_patterns->items; >>>> + struct ufid_to_rte_flow_data *flow_data = NULL; >>>> + bool actions_offloaded = true; >>>> struct rte_flow *flow = NULL; >>>> struct rte_flow_error error; >>>> - int ret; >>>> >>>> - ret = parse_flow_actions(netdev, &actions, nl_actions, actions_len); >>>> - if (ret) { >>>> - goto out; >>>> + if (enable_full) { >>>> + flow = netdev_offload_dpdk_flow_create(netdev, &flow_attr, items, >>>> + flow_actions, &error); >>>> } >>>> - flow = netdev_offload_dpdk_flow_create(netdev, &flow_attr, patterns->items, >>>> - &actions, &error); >>>> -out: >>>> - free_flow_actions(&actions); >>>> - return flow; >>>> + >>>> + if (!flow) { >>>> + /* If we failed to offload the rule actions fallback to MARK+RSS >>>> + * actions. >>>> + */ >>> A debug message might be useful here, when we fallback to mark/rss action ? >> We can, but this is just a refactor commit, and this fallback is not >> new. Add it anyway? > I think it'd be useful to add a debug message here and also in > parse_flow_actions() to indicate that action offloading failed. For the fallback, we have this info by dpctl/dump-flows ("partial"). Furthermore, it may flood the log, depending on PMD rte_flow support. For parse_flow_action failure, there are some prints there with the places of failure, to be more useful rather than just a generic failure message. Let's keep this commit as a refactor commit without any logic changes, that can be added later. What do you think? > >>>> + actions_offloaded = false; >>>> + flow_attr.transfer = 0; >>>> + add_flow_mark_rss_actions(&rss_actions, flow_mark, netdev); >>>> + flow = netdev_offload_dpdk_flow_create(netdev, &flow_attr, items, >>>> + &rss_actions, &error); >>>> + } >>>> + >>>> + if (flow) { >>>> + flow_data = ufid_to_rte_flow_associate(ufid, netdev, flow, >>>> + actions_offloaded); >>>> + VLOG_DBG("%s: installed flow %p by ufid "UUID_FMT, >>>> + netdev_get_name(netdev), flow, >>>> + UUID_ARGS((struct uuid *) ufid)); >>>> + } >>>> + >>>> + free_flow_actions(&rss_actions); >>> This free is needed only when we fallback to mark/rss offload, not otherwise. >> OK. >>>> + return flow_data; >>>> } >>>> >>>> static struct ufid_to_rte_flow_data * >>>> @@ -1541,9 +1538,9 @@ netdev_offload_dpdk_add_flow(struct netdev *netdev, >>>> struct offload_info *info) >>>> { >>>> struct flow_patterns patterns = { .items = NULL, .cnt = 0 }; >>>> - struct ufid_to_rte_flow_data *flows_data = NULL; >>>> - bool actions_offloaded = true; >>>> - struct rte_flow *flow; >>>> + struct flow_actions actions = { .actions = NULL, .cnt = 0 }; >>>> + struct ufid_to_rte_flow_data *flow_data = NULL; >>>> + int err; >>>> >>>> if (parse_flow_match(&patterns, match)) { >>>> VLOG_DBG_RL(&rl, "%s: matches of ufid "UUID_FMT" are not supported", >>>> @@ -1551,28 +1548,15 @@ netdev_offload_dpdk_add_flow(struct netdev *netdev, >>>> goto out; >>>> } >>>> >>>> - flow = netdev_offload_dpdk_actions(netdev, &patterns, nl_actions, >>>> - actions_len); >>>> - if (!flow) { >>>> - /* If we failed to offload the rule actions fallback to MARK+RSS >>>> - * actions. >>>> - */ >>>> - flow = netdev_offload_dpdk_mark_rss(&patterns, netdev, >>>> - info->flow_mark); >>>> - actions_offloaded = false; >>>> - } >>>> + err = parse_flow_actions(netdev, &actions, nl_actions, actions_len); >>>> >>>> - if (!flow) { >>>> - goto out; >>>> - } >>>> - flows_data = ufid_to_rte_flow_associate(ufid, netdev, flow, >>>> - actions_offloaded); >>>> - VLOG_DBG("%s: installed flow %p by ufid "UUID_FMT, >>>> - netdev_get_name(netdev), flow, UUID_ARGS((struct uuid *)ufid)); >>>> + flow_data = create_netdev_offload(netdev, ufid, &patterns, &actions, !err, >>>> + info->flow_mark); >>>> >>>> out: >>>> free_flow_patterns(&patterns); >>>> - return flows_data; >>>> + free_flow_actions(&actions); >>>> + return flow_data; >>>> } >>>> >>>> static int >>>> -- >>>> 2.28.0.546.g385c171 >>>>
On Thu, Feb 25, 2021 at 7:51 PM Eli Britstein <elibr@nvidia.com> wrote: > > > On 2/25/2021 9:35 AM, Sriharsha Basavapatna wrote: > > On Wed, Feb 24, 2021 at 3:47 PM Eli Britstein <elibr@nvidia.com> wrote: > >> > >> On 2/24/2021 11:55 AM, Sriharsha Basavapatna wrote: > >>> On Wed, Feb 10, 2021 at 8:57 PM Eli Britstein <elibr@nvidia.com> wrote: > >>>> Refactor offload rule creation as a pre-step towards tunnel matching > >>>> that depend on the netdev. > >>>> > >>>> Signed-off-by: Eli Britstein <elibr@nvidia.com> > >>>> Reviewed-by: Gaetan Rivet <gaetanr@nvidia.com> > >>>> --- > >>>> lib/netdev-offload-dpdk.c | 106 ++++++++++++++++---------------------- > >>>> 1 file changed, 45 insertions(+), 61 deletions(-) > >>>> > >>>> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c > >>>> index 493cc9159..f6e668bff 100644 > >>>> --- a/lib/netdev-offload-dpdk.c > >>>> +++ b/lib/netdev-offload-dpdk.c > >>>> @@ -1009,30 +1009,6 @@ add_flow_mark_rss_actions(struct flow_actions *actions, > >>>> add_flow_action(actions, RTE_FLOW_ACTION_TYPE_END, NULL); > >>>> } > >>>> > >>>> -static struct rte_flow * > >>>> -netdev_offload_dpdk_mark_rss(struct flow_patterns *patterns, > >>>> - struct netdev *netdev, > >>>> - uint32_t flow_mark) > >>>> -{ > >>>> - struct flow_actions actions = { .actions = NULL, .cnt = 0 }; > >>>> - const struct rte_flow_attr flow_attr = { > >>>> - .group = 0, > >>>> - .priority = 0, > >>>> - .ingress = 1, > >>>> - .egress = 0 > >>>> - }; > >>>> - struct rte_flow_error error; > >>>> - struct rte_flow *flow; > >>>> - > >>>> - add_flow_mark_rss_actions(&actions, flow_mark, netdev); > >>>> - > >>>> - flow = netdev_offload_dpdk_flow_create(netdev, &flow_attr, patterns->items, > >>>> - &actions, &error); > >>>> - > >>>> - free_flow_actions(&actions); > >>>> - return flow; > >>>> -} > >>>> - > >>>> static void > >>>> add_count_action(struct flow_actions *actions) > >>>> { > >>>> @@ -1509,27 +1485,48 @@ parse_flow_actions(struct netdev *netdev, > >>>> return 0; > >>>> } > >>>> > >>>> -static struct rte_flow * > >>>> -netdev_offload_dpdk_actions(struct netdev *netdev, > >>>> - struct flow_patterns *patterns, > >>>> - struct nlattr *nl_actions, > >>>> - size_t actions_len) > >>>> +static struct ufid_to_rte_flow_data * > >>>> +create_netdev_offload(struct netdev *netdev, > >>>> + const ovs_u128 *ufid, > >>>> + struct flow_patterns *flow_patterns, > >>>> + struct flow_actions *flow_actions, > >>>> + bool enable_full, > >>>> + uint32_t flow_mark) > >>>> { > >>>> - const struct rte_flow_attr flow_attr = { .ingress = 1, .transfer = 1 }; > >>>> - struct flow_actions actions = { .actions = NULL, .cnt = 0 }; > >>>> + struct rte_flow_attr flow_attr = { .ingress = 1, .transfer = 1, }; > >>>> + struct flow_actions rss_actions = { .actions = NULL, .cnt = 0 }; > >>>> + struct rte_flow_item *items = flow_patterns->items; > >>>> + struct ufid_to_rte_flow_data *flow_data = NULL; > >>>> + bool actions_offloaded = true; > >>>> struct rte_flow *flow = NULL; > >>>> struct rte_flow_error error; > >>>> - int ret; > >>>> > >>>> - ret = parse_flow_actions(netdev, &actions, nl_actions, actions_len); > >>>> - if (ret) { > >>>> - goto out; > >>>> + if (enable_full) { > >>>> + flow = netdev_offload_dpdk_flow_create(netdev, &flow_attr, items, > >>>> + flow_actions, &error); > >>>> } > >>>> - flow = netdev_offload_dpdk_flow_create(netdev, &flow_attr, patterns->items, > >>>> - &actions, &error); > >>>> -out: > >>>> - free_flow_actions(&actions); > >>>> - return flow; > >>>> + > >>>> + if (!flow) { > >>>> + /* If we failed to offload the rule actions fallback to MARK+RSS > >>>> + * actions. > >>>> + */ > >>> A debug message might be useful here, when we fallback to mark/rss action ? > >> We can, but this is just a refactor commit, and this fallback is not > >> new. Add it anyway? > > I think it'd be useful to add a debug message here and also in > > parse_flow_actions() to indicate that action offloading failed. > > For the fallback, we have this info by dpctl/dump-flows ("partial"). > Furthermore, it may flood the log, depending on PMD rte_flow support. > > For parse_flow_action failure, there are some prints there with the > places of failure, to be more useful rather than just a generic failure > message. > > Let's keep this commit as a refactor commit without any logic changes, > that can be added later. What do you think? Ok. > > > > >>>> + actions_offloaded = false; > >>>> + flow_attr.transfer = 0; > >>>> + add_flow_mark_rss_actions(&rss_actions, flow_mark, netdev); > >>>> + flow = netdev_offload_dpdk_flow_create(netdev, &flow_attr, items, > >>>> + &rss_actions, &error); > >>>> + } > >>>> + > >>>> + if (flow) { > >>>> + flow_data = ufid_to_rte_flow_associate(ufid, netdev, flow, > >>>> + actions_offloaded); > >>>> + VLOG_DBG("%s: installed flow %p by ufid "UUID_FMT, > >>>> + netdev_get_name(netdev), flow, > >>>> + UUID_ARGS((struct uuid *) ufid)); > >>>> + } > >>>> + > >>>> + free_flow_actions(&rss_actions); > >>> This free is needed only when we fallback to mark/rss offload, not otherwise. > >> OK. > >>>> + return flow_data; > >>>> } > >>>> > >>>> static struct ufid_to_rte_flow_data * > >>>> @@ -1541,9 +1538,9 @@ netdev_offload_dpdk_add_flow(struct netdev *netdev, > >>>> struct offload_info *info) > >>>> { > >>>> struct flow_patterns patterns = { .items = NULL, .cnt = 0 }; > >>>> - struct ufid_to_rte_flow_data *flows_data = NULL; > >>>> - bool actions_offloaded = true; > >>>> - struct rte_flow *flow; > >>>> + struct flow_actions actions = { .actions = NULL, .cnt = 0 }; > >>>> + struct ufid_to_rte_flow_data *flow_data = NULL; > >>>> + int err; > >>>> > >>>> if (parse_flow_match(&patterns, match)) { > >>>> VLOG_DBG_RL(&rl, "%s: matches of ufid "UUID_FMT" are not supported", > >>>> @@ -1551,28 +1548,15 @@ netdev_offload_dpdk_add_flow(struct netdev *netdev, > >>>> goto out; > >>>> } > >>>> > >>>> - flow = netdev_offload_dpdk_actions(netdev, &patterns, nl_actions, > >>>> - actions_len); > >>>> - if (!flow) { > >>>> - /* If we failed to offload the rule actions fallback to MARK+RSS > >>>> - * actions. > >>>> - */ > >>>> - flow = netdev_offload_dpdk_mark_rss(&patterns, netdev, > >>>> - info->flow_mark); > >>>> - actions_offloaded = false; > >>>> - } > >>>> + err = parse_flow_actions(netdev, &actions, nl_actions, actions_len); > >>>> > >>>> - if (!flow) { > >>>> - goto out; > >>>> - } > >>>> - flows_data = ufid_to_rte_flow_associate(ufid, netdev, flow, > >>>> - actions_offloaded); > >>>> - VLOG_DBG("%s: installed flow %p by ufid "UUID_FMT, > >>>> - netdev_get_name(netdev), flow, UUID_ARGS((struct uuid *)ufid)); > >>>> + flow_data = create_netdev_offload(netdev, ufid, &patterns, &actions, !err, > >>>> + info->flow_mark); > >>>> > >>>> out: > >>>> free_flow_patterns(&patterns); > >>>> - return flows_data; > >>>> + free_flow_actions(&actions); > >>>> + return flow_data; > >>>> } > >>>> > >>>> static int > >>>> -- > >>>> 2.28.0.546.g385c171 > >>>>
diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c index 493cc9159..f6e668bff 100644 --- a/lib/netdev-offload-dpdk.c +++ b/lib/netdev-offload-dpdk.c @@ -1009,30 +1009,6 @@ add_flow_mark_rss_actions(struct flow_actions *actions, add_flow_action(actions, RTE_FLOW_ACTION_TYPE_END, NULL); } -static struct rte_flow * -netdev_offload_dpdk_mark_rss(struct flow_patterns *patterns, - struct netdev *netdev, - uint32_t flow_mark) -{ - struct flow_actions actions = { .actions = NULL, .cnt = 0 }; - const struct rte_flow_attr flow_attr = { - .group = 0, - .priority = 0, - .ingress = 1, - .egress = 0 - }; - struct rte_flow_error error; - struct rte_flow *flow; - - add_flow_mark_rss_actions(&actions, flow_mark, netdev); - - flow = netdev_offload_dpdk_flow_create(netdev, &flow_attr, patterns->items, - &actions, &error); - - free_flow_actions(&actions); - return flow; -} - static void add_count_action(struct flow_actions *actions) { @@ -1509,27 +1485,48 @@ parse_flow_actions(struct netdev *netdev, return 0; } -static struct rte_flow * -netdev_offload_dpdk_actions(struct netdev *netdev, - struct flow_patterns *patterns, - struct nlattr *nl_actions, - size_t actions_len) +static struct ufid_to_rte_flow_data * +create_netdev_offload(struct netdev *netdev, + const ovs_u128 *ufid, + struct flow_patterns *flow_patterns, + struct flow_actions *flow_actions, + bool enable_full, + uint32_t flow_mark) { - const struct rte_flow_attr flow_attr = { .ingress = 1, .transfer = 1 }; - struct flow_actions actions = { .actions = NULL, .cnt = 0 }; + struct rte_flow_attr flow_attr = { .ingress = 1, .transfer = 1, }; + struct flow_actions rss_actions = { .actions = NULL, .cnt = 0 }; + struct rte_flow_item *items = flow_patterns->items; + struct ufid_to_rte_flow_data *flow_data = NULL; + bool actions_offloaded = true; struct rte_flow *flow = NULL; struct rte_flow_error error; - int ret; - ret = parse_flow_actions(netdev, &actions, nl_actions, actions_len); - if (ret) { - goto out; + if (enable_full) { + flow = netdev_offload_dpdk_flow_create(netdev, &flow_attr, items, + flow_actions, &error); } - flow = netdev_offload_dpdk_flow_create(netdev, &flow_attr, patterns->items, - &actions, &error); -out: - free_flow_actions(&actions); - return flow; + + if (!flow) { + /* If we failed to offload the rule actions fallback to MARK+RSS + * actions. + */ + actions_offloaded = false; + flow_attr.transfer = 0; + add_flow_mark_rss_actions(&rss_actions, flow_mark, netdev); + flow = netdev_offload_dpdk_flow_create(netdev, &flow_attr, items, + &rss_actions, &error); + } + + if (flow) { + flow_data = ufid_to_rte_flow_associate(ufid, netdev, flow, + actions_offloaded); + VLOG_DBG("%s: installed flow %p by ufid "UUID_FMT, + netdev_get_name(netdev), flow, + UUID_ARGS((struct uuid *) ufid)); + } + + free_flow_actions(&rss_actions); + return flow_data; } static struct ufid_to_rte_flow_data * @@ -1541,9 +1538,9 @@ netdev_offload_dpdk_add_flow(struct netdev *netdev, struct offload_info *info) { struct flow_patterns patterns = { .items = NULL, .cnt = 0 }; - struct ufid_to_rte_flow_data *flows_data = NULL; - bool actions_offloaded = true; - struct rte_flow *flow; + struct flow_actions actions = { .actions = NULL, .cnt = 0 }; + struct ufid_to_rte_flow_data *flow_data = NULL; + int err; if (parse_flow_match(&patterns, match)) { VLOG_DBG_RL(&rl, "%s: matches of ufid "UUID_FMT" are not supported", @@ -1551,28 +1548,15 @@ netdev_offload_dpdk_add_flow(struct netdev *netdev, goto out; } - flow = netdev_offload_dpdk_actions(netdev, &patterns, nl_actions, - actions_len); - if (!flow) { - /* If we failed to offload the rule actions fallback to MARK+RSS - * actions. - */ - flow = netdev_offload_dpdk_mark_rss(&patterns, netdev, - info->flow_mark); - actions_offloaded = false; - } + err = parse_flow_actions(netdev, &actions, nl_actions, actions_len); - if (!flow) { - goto out; - } - flows_data = ufid_to_rte_flow_associate(ufid, netdev, flow, - actions_offloaded); - VLOG_DBG("%s: installed flow %p by ufid "UUID_FMT, - netdev_get_name(netdev), flow, UUID_ARGS((struct uuid *)ufid)); + flow_data = create_netdev_offload(netdev, ufid, &patterns, &actions, !err, + info->flow_mark); out: free_flow_patterns(&patterns); - return flows_data; + free_flow_actions(&actions); + return flow_data; } static int