Regards _Sugesh
> -----Original Message----- > From: [email protected] [mailto:ovs-dev- > [email protected]] On Behalf Of Yuanhan Liu > Sent: Tuesday, September 5, 2017 10:23 AM > To: [email protected] > Subject: [ovs-dev] [PATCH v2 1/8] dpif-netdev: associate flow with a mark id > > 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? > + > /* 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. > + cmap_remove(&pmd->mark_to_flow, mark_node, flow->mark); > + flow->has_mark = false; [Sugesh] Where is the hardware flow delete happening? > + } > flow->dead = true; > > dp_netdev_flow_unref(flow); > @@ -2235,6 +2249,38 @@ dp_netdev_pmd_find_flow(const struct > dp_netdev_pmd_thread *pmd, > return NULL; > } > > +static struct dp_netdev_flow * > +dp_netdev_pmd_find_flow_by_mark(const struct > dp_netdev_pmd_thread *pmd, > + const uint32_t mark) { > + struct dp_netdev_flow *netdev_flow; > + > + CMAP_FOR_EACH_WITH_HASH (netdev_flow, mark_node, mark, > + &pmd->mark_to_flow) { > + if (netdev_flow->has_mark && netdev_flow->mark == mark) { > + return netdev_flow; > + } > + } > + > + return NULL; > +} > + > +static bool > +dp_netdev_alloc_flow_mark(const struct dp_netdev_pmd_thread *pmd, > + uint32_t *mark) { > + uint32_t i; > + > + for (i = 0; i < UINT32_MAX; i++) { > + if (!dp_netdev_pmd_find_flow_by_mark(pmd, i)) { > + *mark = i; > + return true; > + } > + } > + > + return false; > +} > + > static void > get_dpif_flow_stats(const struct dp_netdev_flow *netdev_flow_, > struct dpif_flow_stats *stats) @@ -2434,6 +2480,7 @@ > dp_netdev_flow_add(struct dp_netdev_pmd_thread *pmd, > struct dp_netdev_flow *flow; > struct netdev_flow_key mask; > struct dpcls *cls; > + struct offload_info info; > > /* Make sure in_port is exact matched before we read it. */ > ovs_assert(match->wc.masks.in_port.odp_port == ODPP_NONE); @@ - > 2460,6 +2507,7 @@ dp_netdev_flow_add(struct dp_netdev_pmd_thread > *pmd, > memset(&flow->stats, 0, sizeof flow->stats); > flow->dead = false; > flow->batch = NULL; > + flow->has_mark = false; > *CONST_CAST(unsigned *, &flow->pmd_id) = pmd->core_id; > *CONST_CAST(struct flow *, &flow->flow) = match->flow; > *CONST_CAST(ovs_u128 *, &flow->ufid) = *ufid; @@ -2475,6 +2523,22 > @@ dp_netdev_flow_add(struct dp_netdev_pmd_thread *pmd, > cmap_insert(&pmd->flow_table, CONST_CAST(struct cmap_node *, > &flow->node), > dp_netdev_flow_hash(&flow->ufid)); > > + 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. > + flow->has_mark = true; > + flow->mark = info.flow_mark; > + cmap_insert(&pmd->mark_to_flow, > + CONST_CAST(struct cmap_node *, &flow->mark_node), > + info.flow_mark); > + VLOG_INFO("installed flow %p with mark %u\n", flow, flow->mark); > + } > + } > + > if (OVS_UNLIKELY(!VLOG_DROP_DBG((&upcall_rl)))) { > struct ds ds = DS_EMPTY_INITIALIZER; > struct ofpbuf key_buf, mask_buf; @@ -2562,6 +2626,25 @@ > flow_put_on_pmd(struct dp_netdev_pmd_thread *pmd, > old_actions = dp_netdev_flow_get_actions(netdev_flow); > ovsrcu_set(&netdev_flow->actions, new_actions); > > + if (netdev_flow->has_mark) { > + struct offload_info info; > + struct dp_netdev_port *port; > + odp_port_t in_port; > + int ret; > + > + in_port = netdev_flow->flow.in_port.odp_port; > + port = dp_netdev_lookup_port(pmd->dp, in_port); > + info.flow_mark = netdev_flow->mark; > + ret = netdev_flow_put(port->netdev, match, > + CONST_CAST(struct nlattr *, > + put->actions), > + put->actions_len, ufid, &info, > + stats); > + > + VLOG_INFO("%s to modify flow %p associated with mark %u\n", > + ret == 0 ? "succeed" : "failed", > + netdev_flow, netdev_flow->mark); > + } > + > if (stats) { > get_dpif_flow_stats(netdev_flow, stats); > } > @@ -4504,6 +4587,7 @@ dp_netdev_configure_pmd(struct > dp_netdev_pmd_thread *pmd, struct dp_netdev *dp, > ovs_mutex_init(&pmd->flow_mutex); > ovs_mutex_init(&pmd->port_mutex); > cmap_init(&pmd->flow_table); > + cmap_init(&pmd->mark_to_flow); > cmap_init(&pmd->classifiers); > pmd->next_optimization = time_msec() + > DPCLS_OPTIMIZATION_INTERVAL; > pmd->rxq_interval = time_msec() + PMD_RXQ_INTERVAL_LEN; @@ - > 4538,6 +4622,7 @@ dp_netdev_destroy_pmd(struct > dp_netdev_pmd_thread *pmd) > } > cmap_destroy(&pmd->classifiers); > cmap_destroy(&pmd->flow_table); > + cmap_destroy(&pmd->mark_to_flow); > ovs_mutex_destroy(&pmd->flow_mutex); > latch_destroy(&pmd->exit_latch); > seq_destroy(pmd->reload_seq); > diff --git a/lib/netdev.h b/lib/netdev.h index f8482f7..2003165 100644 > --- a/lib/netdev.h > +++ b/lib/netdev.h > @@ -188,6 +188,12 @@ void netdev_send_wait(struct netdev *, int qid); > struct offload_info { > const struct dpif_class *dpif_class; > ovs_be16 tp_dst_port; /* Destination port for tunnel in SET action */ > + > + /* > + * The flow mark id assigened to the flow. If any pkts hit the flow, > + * it will be in the pkt meta data. > + */ > + uint32_t flow_mark; > }; > struct dpif_class; > struct netdev_flow_dump; > -- > 2.7.4 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
