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

[Darrell Ball]

On 9/11/17, 1:04 AM, "ovs-dev-bounces at openvswitch.org on behalf of Yuanhan Liu" <ovs-dev-bounces at openvswitch.org on behalf of yliu at fridaylinux.org> wrote:

    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 <fc at napatech.com>
    > > Signed-off-by: Yuanhan Liu <yliu at fridaylinux.org>
    > > Signed-off-by: Finn Christensen <fc at napatech.com>
    > > ---
    > >  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.


[Darrell] Pls verify, but I thought you held the flow_mutex during create/delete.

    
    [...]
    > > +    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
    dev at openvswitch.org
    https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=SxSo6-I3zZTRdns-rYk4h9OLFx1BNBJFuJMPg9ALn6w&s=wrMA8ThZTWY-xuZQdO83DhWwloYLkD4FrOu2OQXoh40&e=