diff mbox series

[ovs-dev,V2,11/14] netdev-offload-dpdk: Refactor offload rule creation

Message ID 20210210152702.4898-12-elibr@nvidia.com
State New
Headers show
Series Netdev vxlan-decap offload | expand

Commit Message

Eli Britstein Feb. 10, 2021, 3:26 p.m. UTC
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(-)

Comments

Sriharsha Basavapatna Feb. 24, 2021, 9:55 a.m. UTC | #1
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
>
Eli Britstein Feb. 24, 2021, 10:17 a.m. UTC | #2
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
>>
Sriharsha Basavapatna Feb. 25, 2021, 7:35 a.m. UTC | #3
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
> >>
Eli Britstein Feb. 25, 2021, 2:21 p.m. UTC | #4
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
>>>>
Sriharsha Basavapatna Feb. 25, 2021, 5:19 p.m. UTC | #5
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 mbox series

Patch

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