On Tue, May 14, 2013 at 07:29:34PM -0700, Ethan Jackson wrote:
> Upon close inspection, it appears that it's not possible for
> actions to differ between subfacets belonging to a given facet.
> Given this fact, it makes sense to move datapath actions from
> subfacets to their parent facets. It's both conceptually more
> straightforward, and necessary for future threading and megaflow
> work.
>
> Justin Pettit fixed a serious learning bug.
Would you describe it a little? It's a big patch so the fix isn't
necessarily obvious (I haven't read on yet). Is it something that we
should break out separately so that we can backport it?
> Co-authored-by: Justin Pettit <jpettit at nicira.com>
> Signed-off-by: Ethan Jackson <ethan at nicira.com>
I agree with Justin's comment.
subfacet_create() refers to subfacet_make_actions() in a comment, but
there is no longer such a function.
handle_flow_miss_with_facet() previously translated the actions
separately for each packet, if it was necessary due to slow-pathing.
The new version doesn't do that. Are you sure that every packet for a
facet always has the same datapath actions, even if the facet is
slow-pathed? We have a comment in facet_check_consistency() that says
that they can vary:
/* The actions for slow-path flows may legitimately vary from one
* packet to the next. We're done. */
The new odp_actions_stub in struct facet makes facets huge! This
should not be necessary. I'd use a much smaller stub there, if we
need one at all. I would consider an approach like the following.
- Put a pointer, a length, and a small fixed-size buffer that
we think will cover the common case (maybe 32 bytes?) into
struct facet. This avoids not just a huge stub there but
the pretty-large struct ofpbuf (currently 52 bytes on
32-bit) too.
struct nlattr *odp_actions;
size_t odp_actions_len; /* or unsigned int */
uint64_t odp_actions_stub[32 / 8];
- Translate the actions into a larger (1 kB or 4 kB) on-stack
stub in facet_create().
- If the translated actions fit into facet->odp_actions_stub,
use it and point facet->odp_actions to it, otherwise
xmemdup() an allocated region.
- When we're done with the facet, if facet->odp_actions !=
facet->odp_actions_stub, free(facet->odp_actions).
Finally, I think we might actually have a fairly long-standing bug due
to a bad assumption. We assume that a subfacet, that is, a given ODP
key, can map to exactly one facet (see "This shouldn't happen."
comment in subfacet_create()) but now I think that's wrong. For
example, suppose as a thought experiment that the kernel reported no
fields at all for any packet. Then every packet would have the same
ODP key, and so all of them would have the same subfacet, but
obviously we'd want more than one facet. I think that essentially the
same thing, but more subtle, can come up whenever the kernel supports
less-specific matching than userspace. One simple solution seems to
be to just never create facets or subfacets when ODP_FIT_TOO_LITTLE
comes up. That would simplify other code, too, since we could get rid
of SLOW_MATCH, subfacet_slow_reason(), etc.
I'd like to review this again after revision.
Thanks,
Ben.