[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH net RFC] xen-netback: Fix grant ref resolution in RX path

On 13/05/14 17:13, Eric Dumazet wrote:
On Tue, 2014-05-13 at 15:31 +0100, Zoltan Kiss wrote:
The original series for reintroducing grant mapping for netback had a patch [1]
to handle receiving of packets from an another VIF. Grant copy on the receiving
side needs the grant ref of the page to set up the op.
The original patch assumed (wrongly) that the frags array haven't changed. In
the case reported by Sander, the sending guest sent a packet where the linear
buffer and the first frag were under PKT_PROT_LEN (=128) bytes.
xenvif_tx_submit() then pulled up the linear area to 128 bytes, and ditched the
first frag. The receiving side had an off-by-one problem when gathered the grant
This patch fixes that by checking whether the actual frag's page pointer is the
same as the page in the original frag list. It can handle any kind of changes on
the original frags array, like:
- removing granted frags from the beginning or the end
- adding local pages to the frags list
To keep it optimized to the most common cases, it doesn't handle when the order
of the original frags changed. That would require ubuf to be reseted to the
beginning of the chain (skb_shinfo(skb)->destructor_arg), and reiterating
through the list every time.

- Is it a safe assumption that nothing changes the order of the original frags?
   Removing them from the array or injecting new pages anywhere is not a 
- I used UINT_MAX as a kind of INVALID_GRANT_REF, however there is no such thing
   in the grant mapping API. Should we codify this or is it better if we just
   find another way to distinguish whether a frag is local or not?
- Should this fix go to David's net tree or directly to the mainline tree? Or

[1]: 3e2234: xen-netback: Handle foreign mapped pages on the guest RX path

Reported-by: Sander Eikelenboom <linux@xxxxxxxxxxxxxx>
Signed-off-by: Zoltan Kiss <zoltan.kiss@xxxxxxxxxx>

The 'cleanup' of stale ubufs should be right after __pskb_pull_tail().
We can't fix every place in the kernel where frags might be changed, especially with a netback specific stuff, so unfortunately that won't work

This is the function that can 'consume frags' after all.

Its not clear that you catch all cases, like skbs being purged in case
of device dismantle.
We need this list for two reason:
a) give back the pages to the sending guest (kfree/skb_copy_ubufs)
b) find out the grant refs when the skb is sent to another vif
b) is handled by this patch. For a) netback doesn't mind if granted frags were removed and/or local ones were injected. It only needs to give back the pages, it doesn't matter how the skb ended up. The only other problematic point if frags are passed around between skbs, I'll write a separate mail about it.

I am not saying your patch is wrong, only that it adds yet an obscure
thing with no comments. In two years, nobody will understand this.
I agree, in v3 there will be more comment lines than actual new code :)

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.