lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Mon, 14 Dec 2009 11:59:57 -0800 From: Shirley Ma <mashirle@...ibm.com> To: "Michael S. Tsirkin" <mst@...hat.com> Cc: Rusty Russell <rusty@...tcorp.com.au>, Avi Kivity <avi@...hat.com>, netdev@...r.kernel.org, kvm@...r.kernel.org, linux-kernel@...r.kernel.org, Anthony Liguori <anthony@...emonkey.ws> Subject: Re: [PATCH v2 0/4] Defer skb allocation for both mergeable buffers and big packets in virtio_net 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 netdev" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists