[ovs-dev] [RFC PATCH 1/4] dpif-netdev: Refactor dp_netdev_flow_offload_put()

[Sriharsha Basavapatna]

Hi Eli,

Thanks for your comments, please see my response inline, in the
respective patches.

Thanks,
-Harsha

On Wed, May 6, 2020 at 5:56 PM Eli Britstein <elibr at mellanox.com> wrote:
>
>
> On 4/24/2020 11:23 AM, Sriharsha Basavapatna wrote:
> > This patch refactors dp_netdev_flow_offload_put() to prepare for
> > more changes to support partial action offload, in subsequent patches.
> >
> > - Add a wrapper function to allocate flow-mark.
> > - Move netdev_ports_get() to before flow-mark allocation.
> This is a refactor commit. Moving netdev_ports_get changes logic. Why is
> it needed?

[Harsha] For partial action offload, we need to get the in-port's
netdev-type (vhost) to determine if the flow should be offloaded.
There's no need to allocate 'mark' if it is an egress flow. You can
see how this is useful in Patch-2. Hence netdev_ports_get() is moved
ahead of mark allocation. In addition, to improve readability of
dp_netdev_flow_offload_put(), I have added a wrapper function to
contain the details of mark allocation logic.



> >
> > Signed-off-by: Sriharsha Basavapatna <sriharsha.basavapatna at broadcom.com>
> > ---
> >   lib/dpif-netdev.c | 71 ++++++++++++++++++++++++++++++-----------------
> >   1 file changed, 45 insertions(+), 26 deletions(-)
> >
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > index ea7b187b2..781b233f4 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -2357,6 +2357,43 @@ dp_netdev_flow_offload_del(struct dp_flow_offload_item *offload)
> >       return mark_to_flow_disassociate(offload->pmd, offload->flow);
> >   }
> >
> > +static int
> > +dp_netdev_alloc_flow_mark(struct dp_netdev_flow *flow, bool modification,
> > +                          uint32_t *markp)
> > +{
> > +    uint32_t mark;
> > +
> > +    if (modification) {
> > +        mark = flow->mark;
> > +        ovs_assert(mark != INVALID_FLOW_MARK);
> > +        *markp = mark;
> > +        return 0;
> > +    }
> > +
> > +    /*
> > +     * If a mega flow has already been offloaded (from other PMD
> > +     * instances), do not offload it again.
> > +     */
> > +    mark = megaflow_to_mark_find(&flow->mega_ufid);
> > +    if (mark != INVALID_FLOW_MARK) {
> > +        VLOG_DBG("Flow has already been offloaded with mark %u\n", mark);
> > +        if (flow->mark != INVALID_FLOW_MARK) {
> > +            ovs_assert(flow->mark == mark);
> > +        } else {
> > +            mark_to_flow_associate(mark, flow);
> > +        }
> > +        return 1;
> > +    }
> > +
> > +    mark = netdev_offload_flow_mark_alloc();
> > +    if (mark == INVALID_FLOW_MARK) {
> > +        VLOG_ERR("Failed to allocate flow mark!\n");
> > +    }
> > +
> > +    *markp = mark;
> > +    return 0;
> > +}
> > +
> >   /*
> >    * There are two flow offload operations here: addition and modification.
> >    *
> > @@ -2385,36 +2422,18 @@ dp_netdev_flow_offload_put(struct dp_flow_offload_item *offload)
> >           return -1;
> >       }
> >
> > -    if (modification) {
> > -        mark = flow->mark;
> > -        ovs_assert(mark != INVALID_FLOW_MARK);
> > -    } else {
> > -        /*
> > -         * If a mega flow has already been offloaded (from other PMD
> > -         * instances), do not offload it again.
> > -         */
> > -        mark = megaflow_to_mark_find(&flow->mega_ufid);
> > -        if (mark != INVALID_FLOW_MARK) {
> > -            VLOG_DBG("Flow has already been offloaded with mark %u\n", mark);
> > -            if (flow->mark != INVALID_FLOW_MARK) {
> > -                ovs_assert(flow->mark == mark);
> > -            } else {
> > -                mark_to_flow_associate(mark, flow);
> > -            }
> > -            return 0;
> > -        }
> > +    port = netdev_ports_get(in_port, dpif_type_str);
> > +    if (!port) {
> > +        return -1;
> > +    }
> >
> > -        mark = netdev_offload_flow_mark_alloc();
> > -        if (mark == INVALID_FLOW_MARK) {
> > -            VLOG_ERR("Failed to allocate flow mark!\n");
> > -        }
> > +    if (dp_netdev_alloc_flow_mark(flow, modification, &mark)) {
> > +            /* flow already offloaded */
> > +            netdev_close(port);
> > +            return 0;
> >       }
> >       info.flow_mark = mark;
> >
> > -    port = netdev_ports_get(in_port, dpif_type_str);
> > -    if (!port) {
> > -        goto err_free;
> > -    }
> >       /* Taking a global 'port_mutex' to fulfill thread safety restrictions for
> >        * the netdev-offload-dpdk module. */
> >       ovs_mutex_lock(&pmd->dp->port_mutex);