netback grant copying issues

netback grant copying issues

[Jan Beulich]

Ian, Wei,

I seem to be seeing two issues in the grant copy handling of netback,
solely from code inspection:

1) Shouldn't MAX_GRANT_COPY_OPS, to take care of the copying
the header may require, be
((MAX_SKB_FRAGS + 1) * XEN_NETIF_RX_RING_SIZE)?

2) xenvif_gop_frag_copy() may set up any number of copy_gop-s
per meta slot, yet xenvif_check_gop() checks only one.

Apart from that I find it puzzling that the comment ahead of
xenvif_gop_frag_copy() talks about flipping interfaces, while iirc
all flipping code has long been deleted from the upstream driver.

Jan

Re: netback grant copying issues

[Wei Liu]

On Fri, Sep 04, 2015 at 03:27:42AM -0600, Jan Beulich wrote:
> Ian, Wei,
> 
> I seem to be seeing two issues in the grant copy handling of netback,
> solely from code inspection:
> 
> 1) Shouldn't MAX_GRANT_COPY_OPS, to take care of the copying
> the header may require, be
> ((MAX_SKB_FRAGS + 1) * XEN_NETIF_RX_RING_SIZE)?
> 

Not sure what you mean by "header". I take it you mean SKB itself? We do
need to use grant copy data inside SKB (not in frag list) to frontend.

It sounds plausible that there is some sort of miscounting, just that no
SKB is seen to be so broken to trigger that.

With that in mind, even MAX_SKB_FRAGS + 1 is not enough. It would be
MAX_SKB_FRAGS + 64K / PAGE_SIZE, i.e. we count the most extreme
situation that we have 63K data in SKB, and 1 byte in each frag.

> 2) xenvif_gop_frag_copy() may set up any number of copy_gop-s
> per meta slot, yet xenvif_check_gop() checks only one.
> 

That's indeed something that need to be fixed.

> Apart from that I find it puzzling that the comment ahead of
> xenvif_gop_frag_copy() talks about flipping interfaces, while iirc
> all flipping code has long been deleted from the upstream driver.
> 

That should be removed IMO.

Wei.

> Jan

Re: netback grant copying issues

[Jan Beulich]

>>> On 07.09.15 at 16:47, <wei.liu2@citrix.com> wrote:
> On Fri, Sep 04, 2015 at 03:27:42AM -0600, Jan Beulich wrote:
>> Ian, Wei,
>> 
>> I seem to be seeing two issues in the grant copy handling of netback,
>> solely from code inspection:
>> 
>> 1) Shouldn't MAX_GRANT_COPY_OPS, to take care of the copying
>> the header may require, be
>> ((MAX_SKB_FRAGS + 1) * XEN_NETIF_RX_RING_SIZE)?
>> 
> 
> Not sure what you mean by "header". I take it you mean SKB itself? We do
> need to use grant copy data inside SKB (not in frag list) to frontend.

Yes - with "header" is mean everything up to skb_headlen(skb).

> It sounds plausible that there is some sort of miscounting, just that no
> SKB is seen to be so broken to trigger that.

Why "broken"?

> With that in mind, even MAX_SKB_FRAGS + 1 is not enough. It would be
> MAX_SKB_FRAGS + 64K / PAGE_SIZE, i.e. we count the most extreme
> situation that we have 63K data in SKB, and 1 byte in each frag.

No, every component (head + every frag) can contribute
exactly one copy operation to a RX ring slot (larger heads, just
like larger frags) would get copied into multiple RX ring slots.

Jan

Re: netback grant copying issues

[Wei Liu]

On Mon, Sep 07, 2015 at 09:03:12AM -0600, Jan Beulich wrote:
> >>> On 07.09.15 at 16:47, <wei.liu2@citrix.com> wrote:
> > On Fri, Sep 04, 2015 at 03:27:42AM -0600, Jan Beulich wrote:
> >> Ian, Wei,
> >> 
> >> I seem to be seeing two issues in the grant copy handling of netback,
> >> solely from code inspection:
> >> 
> >> 1) Shouldn't MAX_GRANT_COPY_OPS, to take care of the copying
> >> the header may require, be
> >> ((MAX_SKB_FRAGS + 1) * XEN_NETIF_RX_RING_SIZE)?
> >> 
> > 
> > Not sure what you mean by "header". I take it you mean SKB itself? We do
> > need to use grant copy data inside SKB (not in frag list) to frontend.
> 
> Yes - with "header" is mean everything up to skb_headlen(skb).
> 
> > It sounds plausible that there is some sort of miscounting, just that no
> > SKB is seen to be so broken to trigger that.
> 
> Why "broken"?
> 

Broken because I think that's going to introduce some performance
regression. But I see why "broken" is a bad description. Let's say, a
bit "extreme".

> > With that in mind, even MAX_SKB_FRAGS + 1 is not enough. It would be
> > MAX_SKB_FRAGS + 64K / PAGE_SIZE, i.e. we count the most extreme
> > situation that we have 63K data in SKB, and 1 byte in each frag.
> 
> No, every component (head + every frag) can contribute
> exactly one copy operation to a RX ring slot (larger heads, just
> like larger frags) would get copied into multiple RX ring slots.
> 

I think you misunderstand copy_op slot vs RX ring slot.  The limitation
rs for copy_op slots, not RX ring slots. Note that multiplication of
XEN_NETIF_RX_RING_SIZE.

The skb header data len is not limited to one 4K page (4K because that's
what xen grant table interface supports), netback will break skb header
data into multiple copy_op slots.

Wei.

> Jan

Re: netback grant copying issues

[Jan Beulich]

>>> On 07.09.15 at 17:10, <wei.liu2@citrix.com> wrote:
> On Mon, Sep 07, 2015 at 09:03:12AM -0600, Jan Beulich wrote:
>> >>> On 07.09.15 at 16:47, <wei.liu2@citrix.com> wrote:
>> > With that in mind, even MAX_SKB_FRAGS + 1 is not enough. It would be
>> > MAX_SKB_FRAGS + 64K / PAGE_SIZE, i.e. we count the most extreme
>> > situation that we have 63K data in SKB, and 1 byte in each frag.
>> 
>> No, every component (head + every frag) can contribute
>> exactly one copy operation to a RX ring slot (larger heads, just
>> like larger frags) would get copied into multiple RX ring slots.
>> 
> 
> I think you misunderstand copy_op slot vs RX ring slot.  The limitation
> rs for copy_op slots, not RX ring slots. Note that multiplication of
> XEN_NETIF_RX_RING_SIZE.
> 
> The skb header data len is not limited to one 4K page (4K because that's
> what xen grant table interface supports), netback will break skb header
> data into multiple copy_op slots.

But in that case it will not only use multiple copy_op slots, but also
copy into multiple RX slots. I.e. still - per RX slot there can only be
MAX_SKB_FRAGS+1 copy_op slots.

Two examples may help

head	1 byte
frag1	1 byte
...
fragN	1 byte

will need N+1 copy slots and one RX slot.

head	8k		2 copy slots, 2 RX slots
frag1	1 byte		3rd copy slot, 3rd RX slot
...
fragN	1 byte		N+2nd copy slot, 3rd RX slot

Jan

Re: netback grant copying issues

[Wei Liu]

On Mon, Sep 07, 2015 at 09:23:59AM -0600, Jan Beulich wrote:
> >>> On 07.09.15 at 17:10, <wei.liu2@citrix.com> wrote:
> > On Mon, Sep 07, 2015 at 09:03:12AM -0600, Jan Beulich wrote:
> >> >>> On 07.09.15 at 16:47, <wei.liu2@citrix.com> wrote:
> >> > With that in mind, even MAX_SKB_FRAGS + 1 is not enough. It would be
> >> > MAX_SKB_FRAGS + 64K / PAGE_SIZE, i.e. we count the most extreme
> >> > situation that we have 63K data in SKB, and 1 byte in each frag.
> >> 
> >> No, every component (head + every frag) can contribute
> >> exactly one copy operation to a RX ring slot (larger heads, just
> >> like larger frags) would get copied into multiple RX ring slots.
> >> 
> > 
> > I think you misunderstand copy_op slot vs RX ring slot.  The limitation
> > rs for copy_op slots, not RX ring slots. Note that multiplication of
> > XEN_NETIF_RX_RING_SIZE.
> > 
> > The skb header data len is not limited to one 4K page (4K because that's
> > what xen grant table interface supports), netback will break skb header
> > data into multiple copy_op slots.
> 
> But in that case it will not only use multiple copy_op slots, but also
> copy into multiple RX slots. I.e. still - per RX slot there can only be
> MAX_SKB_FRAGS+1 copy_op slots.
> 
> Two examples may help
> 
> head	1 byte
> frag1	1 byte
> ...
> fragN	1 byte
> 
> will need N+1 copy slots and one RX slot.
> 
> head	8k		2 copy slots, 2 RX slots
> frag1	1 byte		3rd copy slot, 3rd RX slot
> ...
> fragN	1 byte		N+2nd copy slot, 3rd RX slot
> 

Yes, indeed. I missed that. I should have notice this when I wrote "what
xen grant table interface supports".

> Jan

Re: netback grant copying issues

[David Vrabel]

On 04/09/15 10:27, Jan Beulich wrote:
> Ian, Wei,
> 
> I seem to be seeing two issues in the grant copy handling of netback,
> solely from code inspection:
> 
> 1) Shouldn't MAX_GRANT_COPY_OPS, to take care of the copying
> the header may require, be
> ((MAX_SKB_FRAGS + 1) * XEN_NETIF_RX_RING_SIZE)?

We really should have the copy batches be independent of ring size and
skb format.

e.g., Call xenvif_gnt_copy_add(ref, ptr, len) for each copy op needed.
This will issue the hypercall when the batch is full.  There would need
to be axenvif_gnt_copy_flush() at the end for last hypercall for the
remaining ops.

David