On Thu, Jun 16, 2011 at 1:02 PM, Ben Pfaff <[email protected]> wrote: > On Wed, Jun 15, 2011 at 11:00:06AM -0700, Jesse Gross wrote: >> The current implementation of make_writable() is both overly complex >> and unnecessarily aggressive about copying data. We can improve >> performance by only making a copy of the data if someone else holds >> a reference to the portion of the data that we want to modify. This >> means that if a clone is held by the TCP stack for retransmission then >> we do not need to make a copy if we are changing the IP header because >> it will get regenerated on retransmit anyways. Even when it is necessary >> to copy we avoid duplicating struct sk_buff. >> >> Signed-off-by: Jesse Gross <[email protected]> > > This looks like a potentially very useful optimization. Thanks for > doing it. > > I've studied it for a few minutes and I don't see any actual problems. > > I noticed in set_nw_tos() that the variable "f" doesn't help with > anything or make the code easier to read (not a new issue). I think > we should just write nh->tos in place of *f.
You're right, changed. > In do_execute_actions(), I think that the scope of the new 'err' > variable could be limited to the "for" loop. Also changed. > The very final call to kfree_skb() at the end of do_execute_actions() > could possibly be a consume_skb(), since this isn't really an error > (not a new issue either). I think this is a good idea but I'm going to do it in a separate patch because I need to backport consume_skb() and then there a few other call sites that can be converted as well. _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
