[ovs-dev] [netlink v4+3] datapath: Append ODFPT_ACTIONS attribute as fragments.

[Ben Pfaff]

On Mon, Jan 24, 2011 at 09:26:00AM -0800, Jesse Gross wrote:
> On Sun, Jan 23, 2011 at 11:21 AM, Ben Pfaff <blp at nicira.com> wrote:
> > diff --git a/datapath/datapath.c b/datapath/datapath.c
> > index fb8a4c8..28616c8 100644
> > --- a/datapath/datapath.c
> > +++ b/datapath/datapath.c
> > @@ -877,6 +933,21 @@ static int odpfc_fill_info(struct sw_flow *flow, struct datapath *dp,
> > ? ? ? ?if (tcp_flags)
> > ? ? ? ? ? ? ? ?NLA_PUT_U8(skb, ODPFT_TCP_FLAGS, tcp_flags);
> >
> > +
> > + ? ? ? /* Put ODPFT_ACTIONS nested attribute header. */
> > + ? ? ? if (skb_tailroom(skb) < NLA_HDRLEN)
> > + ? ? ? ? ? ? ? goto nla_put_failure;
> > + ? ? ? nla = (struct nlattr *) skb_put(skb, NLA_HDRLEN);
> > + ? ? ? nla->nla_type = ODPFT_ACTIONS;
> > + ? ? ? nla->nla_len = sf_acts->actions_len;
> > +
> > + ? ? ? /* Put ODPFT_ACTIONS contents inline if space, otherwise in frags. */
> > + ? ? ? if (skb_tailroom(skb) >= sf_acts->actions_len)
> > + ? ? ? ? ? ? ? memcpy(__skb_put(skb, sf_acts->actions_len),
> > + ? ? ? ? ? ? ? ? ? ? ?sf_acts->actions, sf_acts->actions_len);
> > + ? ? ? else
> > + ? ? ? ? ? ? ? err = append_frag_data(skb, sf_acts->actions, sf_acts->actions_len);
> 
> I think this is OK but there were a few things that I thought might be
> an issue when looking through this:
> * Once we append a frag to the end, we should no longer continue
> dumping.  This should happen because genlmsg_put() checks
> skb_tailroom(), which always returns 0 if the skb is nonlinear.

OK.

> * The open-coded version of nla_nest_start/end here doesn't do padding
> for alignment.  We don't have to worry about it though because nothing
> can come after us.

Just because it's at the end doesn't mean that the padding is optional.
It is still required as far as I know.  But the data we are adding is
valid Netlink attributes and thus already has enough padding.

I changed the comment to this:

	/* Put ODP_FLOW_ATTR_ACTIONS contents inline if space, otherwise in
	 * frags.  No padding is needed because the actions are Netlink
	 * atttributes themselves and thus already padded properly.  (If we add
	 * frags then this will be the last flow to be dumped; it works out OK
	 * because the next genlmsg_put() checks the skb's tailroom, which is
	 * zero for an skb with frags.)
	 */

> This might also be of interest in the upcall code to avoid higher
> order allocations in GFP_ATOMIC context if we have to pass the actions
> up to userspace.  We could also play some games with the packet data
> itself, especially if it contains frags.  This stuff might not be
> common enough to be worth optimizing (The allocation failure issues
> aren't an optimization but maybe we can hope that people don't have
> huge action lists with sFlow? Or just avoid the need to pass up the
> action list.).

My intent has been to avoid the need to pass up the action list.

> On the other hand, if this stuff is really rare and not worth worrying
> about then maybe we should just use pskb_expand_head() here and save
> the trouble of dealing with frags.

I'm concerned that if we have a few long action lists then we are likely
to have many.  Higher-order allocations are likely to fail, even with
GFP_KERNEL.

A much-nicer optimization would be to allocate large action lists with
the page allocator instead of the slab allocator, then share those pages
into the skbuff frags list and avoid copying data altogether.

I'm testing this patch now, I'll send a followup if changes are needed.