On Sun, Sep 10, 2017 at 04:14:01PM +0000, Chandran, Sugesh wrote:
> > This patch associate a flow with a mark id (a uint32_t number) by CMAP.
> >
> > It re-uses the flow API (more precisely, the ->flow_put method) to setup the
> > hw flow. The flow_put implementation then is supposed to create a flow
> > with MARK action.
> >
> > Co-authored-by: Finn Christensen <[email protected]>
> > Signed-off-by: Yuanhan Liu <[email protected]>
> > Signed-off-by: Finn Christensen <[email protected]>
> > ---
> > lib/dpif-netdev.c | 85
> > +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > lib/netdev.h | 6 ++++
> > 2 files changed, 91 insertions(+)
>
> [snip]
> > * Any thread trying to keep a rule from being freed should hold its
> > own
> > @@ -569,6 +575,8 @@ struct dp_netdev_pmd_thread {
> > struct ovs_mutex flow_mutex;
> > struct cmap flow_table OVS_GUARDED; /* Flow table. */
> >
> > + struct cmap mark_to_flow;
> [Sugesh] I would prefer to have this cmap per port than a pmd. The reason
> being it offers better
> Performance due to less lookup. Anyway the flow programming is on netdev,
> hence its good to
> keep it netdev centric than pmd. What do you think?
I think it might be workable.
> > /* One classifier per in_port polled by the pmd */
> > struct cmap classifiers;
> > /* Periodically sort subtable vectors according to hit frequencies */
> > @@ -
> > 1839,6 +1847,8 @@ dp_netdev_pmd_remove_flow(struct
> > dp_netdev_pmd_thread *pmd,
> > OVS_REQUIRES(pmd->flow_mutex)
> > {
> > struct cmap_node *node = CONST_CAST(struct cmap_node *, &flow-
> > >node);
> > + struct cmap_node *mark_node = CONST_CAST(struct cmap_node *,
> > + &flow->mark_node);
> > struct dpcls *cls;
> > odp_port_t in_port = flow->flow.in_port.odp_port;
> >
> > @@ -1846,6 +1856,10 @@ dp_netdev_pmd_remove_flow(struct
> > dp_netdev_pmd_thread *pmd,
> > ovs_assert(cls != NULL);
> > dpcls_remove(cls, &flow->cr);
> > cmap_remove(&pmd->flow_table, node, dp_netdev_flow_hash(&flow-
> > >ufid));
> > + if (flow->has_mark) {
> [Sugesh] Any reason these are not mutex protected? As far as I know the flow
> remove
> is handled in revalidator thread. Looks like this is a critical section. So
> the install and delete
> should be protected.
I think you are right.
[...]
> > + if (netdev_is_flow_api_enabled() &&
> > + dp_netdev_alloc_flow_mark(pmd, &info.flow_mark)) {
> > + struct dp_netdev_port *port;
> > + port = dp_netdev_lookup_port(pmd->dp, in_port);
> > + if (netdev_flow_put(port->netdev, match,
> > + CONST_CAST(struct nlattr *, actions),
> > + actions_len, ufid, &info, NULL) == 0) {
> [Sugesh] Cost of flow install is a concern here. It is vary for device to
> device.
> Have you done any analysis on the cost of rte_flow translate + hardware flow
> install?
> It would be nice to mention it in the cover letter.
I haven't measured it yet: it's not my current focus. It's not in the
datapath after all. Besides, it might not be a trivial task to optimize
it.
--yliu
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
