[ovs-dev] [PATCH v7 1/6] dpif-netdev: associate flow with a mark id

Shahaf Shuler shahafs at mellanox.com
Tue Mar 20 11:20:52 UTC 2018


Hi Ian,

Thursday, March 15, 2018 1:55 PM, Stokes, Ian:
> Adding Shahaf Shuler
> > > +static void
> > > +try_netdev_flow_put(struct dp_netdev_pmd_thread *pmd,
> odp_port_t
> > in_port,
> > > +                    struct dp_netdev_flow *flow, struct match *match,
> > > +                    const struct nlattr *actions, size_t actions_len) {
> > > +    struct offload_info info;
> > > +    struct dp_netdev_port *port;
> > > +    bool modification = flow->mark != INVALID_FLOW_MARK;
> > > +    const char *op = modification ? "modify" : "add";
> > > +    uint32_t mark;
> > > +    int ret;
> > > +
> > > +    ovs_mutex_lock(&flow_mark.mutex);
> > > +
> > > +    if (modification) {
> > > +        mark = flow->mark;
> > > +    } else {
> > > +        if (!netdev_is_flow_api_enabled()) {
> > > +            goto out;
> > > +        }
> > > +
> > > +        /*
> > > +         * 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);
> > > +            mark_to_flow_associate(mark, flow);
> > > +            goto out;
> > > +        }
> > > +
> > > +        mark = flow_mark_alloc();
> > > +        if (mark == INVALID_FLOW_MARK) {
> > > +            VLOG_ERR("failed to allocate flow mark!\n");
> > > +            goto out;
> > > +        }
> > > +    }
> > > +    info.flow_mark = mark;
> > > +
> > > +    ovs_mutex_lock(&pmd->dp->port_mutex);
> > > +    port = dp_netdev_lookup_port(pmd->dp, in_port);
> > > +    if (!port) {
> > > +        ovs_mutex_unlock(&pmd->dp->port_mutex);
> > > +        goto out;
> > > +    }
> > > +    ret = netdev_flow_put(port->netdev, match,
> > > +                          CONST_CAST(struct nlattr *, actions),
> > > +                          actions_len, &flow->mega_ufid, &info, NULL);
> > > +    ovs_mutex_unlock(&pmd->dp->port_mutex);
> > > +
> > > +    if (ret) {
> > > +        VLOG_ERR("failed to %s netdev flow with mark %u\n", op, mark);
> > > +        if (!modification) {
> > > +            flow_mark_free(mark);
> > > +        } else {
> > > +            mark_to_flow_disassociate(pmd, flow);
> >
> >                In the case here would it not be a call to flush? If
> > the flow could not be modified wouldn't you have to remove all
> > references for it not just on this PMD?

I discussed it with Finn,  

Let's first agree on the case were flow modification succeeds. Please correct me if I am wrong about the flow here. For flow modification each PMD will attempt to modify the flow on its private flow table. 
This will result in multiple flow destroy and creation on the device (netdev_flow_put). After the last PMD finishes each PMD has the modified flow on its private table and the device has one flow with a single mark which was created by the last PMD.

If such behavior is OK, then the error case should also be OK. In case of flow modification error, each PMD will disassociate the flow from its private table. The last PMD which has the last reference for the flow mark will remove it also from the device. Hence there is no need to flush the flow from each PMD upon the first failure. 







More information about the dev mailing list