[ovs-dev] [PATCH v5 1/5] dpif-netdev: associate flow with a mark id
Yuanhan Liu
yliu at fridaylinux.org
Fri Jan 26 08:19:30 UTC 2018
On Wed, Jan 24, 2018 at 05:29:45PM +0000, Stokes, Ian wrote:
> > Meanwhile, the mark to flow mapping becomes to 1:N mapping. That is what
>
> Is this 1:N or 1:1? I thought I spotted below that it's 1:1.
If you look further, you will also spot "1:N".
mark to mega flow is 1:1
mark to flow is 1:N
For why is that, I think the commit log has explained it well.
> > @@ -442,7 +443,9 @@ struct dp_netdev_flow {
> > /* Hash table index by unmasked flow. */
> > const struct cmap_node node; /* In owning dp_netdev_pmd_thread's */
>
> Can you clarify the comment above, at first glance it's not clear /* In owning dp_netdev_pmd_thread's */ ?
Such comment doesn't even come from my patch. It's already there.
> > /* 'flow_table'. */
> > + const struct cmap_node mark_node; /* In owning flow_mark's
> > + mark_to_flow */
>
> Same here, it's not clear what is meant by 'In owning...'
I was following the old comment style. Do you have better suggestions?
>
> Breaks compilation with Sparse. Sparse complains with:
> 'lib/dpif-netdev.c:1860:18: error: symbol 'flow_mark' was not declared. Should it be static?'
Thanks. Will be fixed.
> > +struct flow_mark flow_mark = {
> > + .megaflow_to_mark = CMAP_INITIALIZER,
> > + .mark_to_flow = CMAP_INITIALIZER,
> > + .mutex = OVS_MUTEX_INITIALIZER,
> > +};
> > +
> > +static uint32_t
> > +flow_mark_alloc(void)
> > +{
> > + uint32_t mark;
> > +
>
> I would expect the id pool to be allocated only once before it's used to allocate ids for the marks.
>
> Would it make sense to move the !flow_mark.pool check and allocation out of here to simplify the function to a single purpose and avoid the check for every mark id allocation.
I don't find a good place to put it. Or you have better idea?
> > + if (!flow_mark.pool) {
> > + /* Haven't initiated yet, do it here */
> > + flow_mark.pool = id_pool_create(0, MAX_FLOW_MARK);
> > + }
> > +
> > + if (id_pool_alloc_id(flow_mark.pool, &mark)) {
> > + return mark;
> > + }
> > +
> > + return INVALID_FLOW_MARK;
> > +}
> > +
> > +static void
> > +flow_mark_free(uint32_t mark)
> > +{
> > + id_pool_free_id(flow_mark.pool, mark); }
> > +
> > +/* associate flow with a mark, which is a 1:1 mapping */ static void
>
> Can you specify megaflow in the comment above.
ok.
> > +megaflow_to_mark_associate(const ovs_u128 *mega_ufid, uint32_t mark) {
> > + size_t hash = dp_netdev_flow_hash(mega_ufid);
> > + struct megaflow_to_mark_data *data = xzalloc(sizeof(*data));
> > +
> > + data->mega_ufid = *mega_ufid;
> > + data->mark = mark;
> > +
> > + cmap_insert(&flow_mark.megaflow_to_mark,
> > + CONST_CAST(struct cmap_node *, &data->node), hash); }
> > +
> > +/* disassociate flow with a mark */
> > +static void
> > +megaflow_to_mark_disassociate(const ovs_u128 *mega_ufid) {
> > + size_t hash = dp_netdev_flow_hash(mega_ufid);
> > + struct megaflow_to_mark_data *data;
> > +
> > + CMAP_FOR_EACH_WITH_HASH (data, node, hash,
> > &flow_mark.megaflow_to_mark) {
> > + if (ovs_u128_equals(*mega_ufid, data->mega_ufid)) {
> > + cmap_remove(&flow_mark.megaflow_to_mark,
> > + CONST_CAST(struct cmap_node *, &data->node),
> > hash);
> > + free(data);
> > + return;
> > + }
> > + }
> > +
> > + VLOG_WARN("masked ufid "UUID_FMT" is not associated with a mark?\n",
> > + UUID_ARGS((struct uuid *)mega_ufid)); }
> > +
> > +static inline uint32_t
> > +megaflow_to_mark_find(const ovs_u128 *mega_ufid) {
> > + size_t hash = dp_netdev_flow_hash(mega_ufid);
> > + struct megaflow_to_mark_data *data;
> > +
> > + CMAP_FOR_EACH_WITH_HASH (data, node, hash,
> > &flow_mark.megaflow_to_mark) {
> > + if (ovs_u128_equals(*mega_ufid, data->mega_ufid)) {
> > + return data->mark;
> > + }
> > + }
> > +
> > + return INVALID_FLOW_MARK;
> > +}
> > +
> > +/* associate mark with a flow, which is 1:N mapping */ static void
> > +mark_to_flow_associate(const uint32_t mark, struct dp_netdev_flow
> > +*flow) {
> > + dp_netdev_flow_ref(flow);
> > +
> > + cmap_insert(&flow_mark.mark_to_flow,
> > + CONST_CAST(struct cmap_node *, &flow->mark_node),
> > + mark);
> > + flow->mark = mark;
> > +
> > + VLOG_DBG("associated dp_netdev flow %p with mark %u\n", flow,
> > +mark); }
> > +
>
> I'd like to see a comment explain the purpose of the function below. Which reference is the flow_mark missing?
What do you mean by "which reference is the flow_mark missing"?
--- forgot about this question: I think I understood you after
looked you comments below. Also, you will find the answer
below.
> > +static bool
> > +flow_mark_has_no_ref(uint32_t mark)
> > +{
> > + struct dp_netdev_flow *flow;
> > +
>
> Maybe I'm missing something below, but I expected a hash to be computed for mark before being called with CMAP_FOR_EACH_WITH_HASH?
I treat "mark" as the hash, as it's with the same type of "hash" and it's
uniq. But you are probably right, it might be better to get the hash by
"hash_int". Will fix it.
>
> > + CMAP_FOR_EACH_WITH_HASH (flow, mark_node, mark,
> > + &flow_mark.mark_to_flow) {
> > + if (flow->mark == mark) {
> > + return false;
> > + }
> > + }
> > +
> > + return true;
> > +}
> > +
> > +static int
> > +mark_to_flow_disassociate(struct dp_netdev_pmd_thread *pmd,
> > + struct dp_netdev_flow *flow) {
> > + int ret = 0;
> > + uint32_t mark = flow->mark;
> > + struct cmap_node *mark_node = CONST_CAST(struct cmap_node *,
> > + &flow->mark_node);
> > +
> > + cmap_remove(&flow_mark.mark_to_flow, mark_node, mark);
> > + flow->mark = INVALID_FLOW_MARK;
> > +
>
> A comment on this code block could be helpful. Mainly just to explain how the reference could be missing and the required steps because of this.
It's not about "missing". It's that we are the last reference to the mark,
so that we could remove the mark and the related flow from hardware.
> > + if (flow_mark_has_no_ref(mark)) {
> > + struct dp_netdev_port *port;
> > + odp_port_t in_port = flow->flow.in_port.odp_port;
> > +
>
> Need port Mutex 'pmd->dp->port_mutex' before calling dp_netdev_lookup_port().
Right, will fix it.
>
> > + port = dp_netdev_lookup_port(pmd->dp, in_port);
> > + if (port) {
> > + ret = netdev_flow_del(port->netdev, &flow->mega_ufid, NULL);
> > + }
> > +
> > + flow_mark_free(mark);
> > + VLOG_DBG("freed flow mark %u\n", mark);
> > +
> > + megaflow_to_mark_disassociate(&flow->mega_ufid);
> > + }
> > + dp_netdev_flow_unref(flow);
> > +
> > + return ret;
> > +}
> > +
> > +static void
> > +flow_mark_flush(struct dp_netdev_pmd_thread *pmd) {
> > + struct dp_netdev_flow *flow;
> > +
> > + CMAP_FOR_EACH (flow, mark_node, &flow_mark.mark_to_flow) {
> > + if (flow->pmd_id == pmd->core_id) {
> > + mark_to_flow_disassociate(pmd, flow);
> > + }
> > + }
> > +}
> > +
> > static void
> > dp_netdev_pmd_remove_flow(struct dp_netdev_pmd_thread *pmd,
> > struct dp_netdev_flow *flow) @@ -1845,6 +2015,9
> > @@ 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->mark != INVALID_FLOW_MARK) {
> > + mark_to_flow_disassociate(pmd, flow);
> > + }
> > flow->dead = true;
> >
> > dp_netdev_flow_unref(flow);
> > @@ -2424,6 +2597,87 @@ out:
> > return error;
> > }
> >
> > +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;
> > +
>
> Again port Mutex 'pmd->dp->port_mutex' required in this function for calling dp_netdev_lookup_port().
>
> > + port = dp_netdev_lookup_port(pmd->dp, in_port);
> > + if (!port) {
> > + return;
> > + }
> > +
> > + 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);
> Just to understand this, Am I right in thinking the expeted behavior is a mark esists so it has been offloaded by another PMD, don't offload it in the HW again BUT do mark it as associated. Is this associated call only relevant to the current PMD? (i.e. each PMD must discover this association themselves?)
Yes.
> > + 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;
> > + ret = netdev_flow_put(port->netdev, match,
> > + CONST_CAST(struct nlattr *, actions),
> > + actions_len, &flow->mega_ufid, &info, NULL);
> > + 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);
> > + }
> > + goto out;
> > + }
> > +
> > + if (!modification) {
> > + megaflow_to_mark_associate(&flow->mega_ufid, mark);
> > + mark_to_flow_associate(mark, flow);
> > + }
> > + VLOG_DBG("succeed to %s netdev flow with mark %u\n", op, mark);
>
> The whole section above seems a bit complicated, again comments explaining the behavior that depends on modification would help a lot here.
sure, I will add some explanations.
--yliu
> > +
> > +out:
> > + ovs_mutex_unlock(&flow_mark.mutex);
> > +}
> > +
> > +static void
> > +dp_netdev_get_mega_ufid(const struct match *match, ovs_u128 *mega_ufid)
> > +{
> > + struct flow masked_flow;
> > + size_t i;
> > +
> > + for (i = 0; i < sizeof(struct flow); i++) {
> > + ((uint8_t *)&masked_flow)[i] = ((uint8_t *)&match->flow)[i] &
> > + ((uint8_t *)&match->wc)[i];
> > + }
> > + dpif_flow_hash(NULL, &masked_flow, sizeof(struct flow), mega_ufid);
> > +}
> > +
> > static struct dp_netdev_flow *
> > dp_netdev_flow_add(struct dp_netdev_pmd_thread *pmd,
> > struct match *match, const ovs_u128 *ufid, @@ -2459,12
> > +2713,14 @@ dp_netdev_flow_add(struct dp_netdev_pmd_thread *pmd,
> > memset(&flow->stats, 0, sizeof flow->stats);
> > flow->dead = false;
> > flow->batch = NULL;
> > + flow->mark = INVALID_FLOW_MARK;
> > *CONST_CAST(unsigned *, &flow->pmd_id) = pmd->core_id;
> > *CONST_CAST(struct flow *, &flow->flow) = match->flow;
> > *CONST_CAST(ovs_u128 *, &flow->ufid) = *ufid;
> > ovs_refcount_init(&flow->ref_cnt);
> > ovsrcu_set(&flow->actions, dp_netdev_actions_create(actions,
> > actions_len));
> >
> > + dp_netdev_get_mega_ufid(match, CONST_CAST(ovs_u128 *,
> > + &flow->mega_ufid));
> > netdev_flow_key_init_masked(&flow->cr.flow, &match->flow, &mask);
> >
> > /* Select dpcls for in_port. Relies on in_port to be exact match. */
> > @@ -2474,6 +2730,8 @@ 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));
> >
> > + try_netdev_flow_put(pmd, in_port, flow, match, actions,
> > + actions_len);
> > +
> > if (OVS_UNLIKELY(!VLOG_DROP_DBG((&upcall_rl)))) {
> > struct ds ds = DS_EMPTY_INITIALIZER;
> > struct ofpbuf key_buf, mask_buf; @@ -2554,6 +2812,7 @@
> > flow_put_on_pmd(struct dp_netdev_pmd_thread *pmd,
> > if (put->flags & DPIF_FP_MODIFY) {
> > struct dp_netdev_actions *new_actions;
> > struct dp_netdev_actions *old_actions;
> > + odp_port_t in_port = netdev_flow->flow.in_port.odp_port;
> >
> > new_actions = dp_netdev_actions_create(put->actions,
> > put->actions_len); @@
> > -2561,6 +2820,9 @@ 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);
> >
> > + try_netdev_flow_put(pmd, in_port, netdev_flow, match,
> > + put->actions, put->actions_len);
> > +
> > if (stats) {
> > get_dpif_flow_stats(netdev_flow, stats);
> > }
> > @@ -3570,6 +3832,7 @@ reload_affected_pmds(struct dp_netdev *dp)
> >
> > CMAP_FOR_EACH (pmd, node, &dp->poll_threads) {
> > if (pmd->need_reload) {
> > + flow_mark_flush(pmd);
> > dp_netdev_reload_pmd__(pmd);
> > pmd->need_reload = false;
> > }
> > diff --git a/lib/netdev.h b/lib/netdev.h index 3a545fe..0c1946a 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
More information about the dev
mailing list