> 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.
>
Thanks,
Ok that makes more sense, I wasn't sure was this being dissociated for just
this pmd and its private table so hence a flush could be needed for the other
pmds. But once other pmds will dissociate in the same manner then that is ok.
Ian
>
>
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
