[lkml]   [2009]   [Dec]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [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.


     \ /
      Last update: 2009-12-14 21:03    [W:0.024 / U:9.120 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site