On Sun, 2009-12-13 at 12:19 +0200, Michael S. Tsirkin wrote: > > Shirley, some advice on packaging patches > that I hope will be helpful: > > You did try to split up the patch logically, > and it's better than a single huge patch, but it > can be better. For example, you add static functions > in one patch and use them in another patch, > this works well for new APIs which are documented > so you can understand from the documentation > what function should do, but not for internal, static functions: > one ends up looking at usage, going back to implementation, > back to usage, each time switching between patches. > > One idea on how to split up the patch set better: > - add new "destroy" API and supply documentation > - switch current implementation over to use destroy API > - split current implementation into subfunctions > handling mergeable/big cases > - convert functions to use deferred allocation > [would be nice to convert mergeable/big separately, > but I am not sure this is possible while keeping > patchset bisectable] > > Some suggestions on formatting: > - keep patch names short, and prefix with module name, > not patchset name, so that git summaries look nicer. E.g. > Defer skb allocation -- add destroy buffers function for virtio > Would be better "virtio: add destroy buffers function". > - please supply commit message with some explanation > and motivation that will help someone looking > at git history, without background from mailing list. > E.g. > "Add "destroy" vq API that returns all posted > buffers back to caller. This makes it possible > to avoid tracking buffers in callers to free > them on vq teardown. Will be used by deferred > skb allocation patch.". > - Use "---" to separate description from text, > and generally make patch acceptable to git am. > It is a good idea to use git to generate patches, > for example with git format-patch. > I usually use it with --numbered --thread --cover-letter. > > > Guest virtio_net receives packets from its pre-allocated vring > > buffers, then it delivers these packets to upper layer protocols > > as skb buffs. So it's not necessary to pre-allocate skb for each > > mergable buffer, then frees it when it's useless. > > This patch has deferred skb allocation when receiving packets for > > both big packets and mergeable buffers. It reduces skb > pre-allocations > > and skb_frees. > > E.g. the above should go into commit message for the virtio net > part of patchset. Nice comments, will include them. > I think you need to base your patch on Dave's net-next, > it's too late to put it in 2.6.32, or even 2.6.33. > It also should probably go in through Dave's tree because virtio part > of > patch is very small, while most of it deals with net/virtio_net. > > Tests have been done for small packets, big packets > > and mergeable buffers. > > > > The single netperf TCP_STREAM performance improved for host to > guest. > > It also reduces UDP packets drop rate. > > > BTW, any numbers? Also, 2.6.32 has regressed as compared to 2.6.31. > Did you try with Sridhar Samudrala's patch from net-next applied > that reportedly fixes this? Ok, I will run Dave's net-next tree. Thanks Shirley -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
