Re: [Xen-devel] [PATCH RFC V2] xen/netback: Count ring slots properly when larger MTU sizes are used

[Matt Wilson <msw@xxxxxxxxxx>]

On Thu, Dec 13, 2012 at 11:12:50PM +0000, Palagummi, Siva wrote:
> > -----Original Message-----
> > From: Matt Wilson [mailto:msw@xxxxxxxxxx]
> > Sent: Wednesday, December 12, 2012 3:05 AM
> >
> > On Tue, Dec 11, 2012 at 10:25:51AM +0000, Palagummi, Siva wrote:
> > >
> > > You can clearly see below that copy_off is input to
> > > start_new_rx_buffer while copying frags.
> > 
> > Yes, but that's the right thing to do. copy_off should be set to the
> > destination offset after copying the last byte of linear data, which
> > means "skb_headlen(skb) % PAGE_SIZE" is correct.
> > 
>
> No. It is not correct for two reasons. For example what if
> skb_headlen(skb) is exactly a multiple of PAGE_SIZE. Copy_off would
> be set to ZERO. And now if there exists some data in frags, ZERO
> will be passed in as copy_off value and start_new_rx_buffer will
> return FALSE. And second reason is the obvious case from the current
> code where "offset_in_page(skb->data)" size hole will be left in the
> first buffer after first pass in case remaining data that need to be
> copied is going to overflow the first buffer.

Right, and I'm arguing that having the code leave a hole is less
desirable than potentially increasing the number of copy
operations. I'd like to hear from Ian and others if using the buffers
efficiently is more important than reducing copy operations. Intuitively,
I think it's more important to use the ring efficiently.

> > > So if the buggy "count" calculation below is fixed based on
> > > offset_in_page value then copy_off value also will change
> > > accordingly.
> > 
> > This calculation is not incorrect. You should only need as many
> > PAGE_SIZE buffers as you have linear data to fill.
> > 
>
> This calculation is incorrect and do not match actual slots used as
> it is now unless some new change is done either in nebk_gop_skb or
> in start_new_rx_buffer.

Yes, and I'm proposing to change netbk_gop_skb() and
start_new_rx_buffer().

> > >       linear_len = skb_headlen(skb)
> > >   count = (linear_len <= PAGE_SIZE)
> > >               ? 1
> > >               :DIV_ROUND_UP(offset_in_page(skb->data)+linear_len,
> > PAGE_SIZE));
> > >
> > >       copy_off = ((offset_in_page(skb->data)+linear_len) <
> > 2*PAGE_SIZE)
> > >                   ? linear_len % PAGE_SIZE;
> > >                   : (offset_in_page(skb->data)+linear_len) % PAGE_SIZE;
> > 
> > A change like this makes the code much more difficult to understand.
> > 

> :-) It would have been easier had we written logic using a for loop
> similar to how the counting is done for data in frags. In fact I did
> do mistake in above calculations :-( . A proper logic probably
> should look somewhat like below.

Yes, we tested one version of a fix that modeled the algorithm used in
count_skb_slots() more directly after the copy loop in netbk_gop_skb()
to avoid such errors.

> linear_len=skb_headlen(skb);
> count = (linear_len <= PAGE_SIZE)
>       ? 1
>       :DIV_ROUND_UP(offset_in_page(skb->data)+linear_len,PAGE_SIZE);
> 
> copy_off = (linear_len <= PAGE_SIZE)
>               ?linear_len
>               :( offset_in_page(skb->data)+linear_len -1)%PAGE_SIZE+1;

If we can agree that utilizing buffers efficiently is more important
than reducing copy operations, we can avoid this complexity.

> > For some SKBs with large linear buffers, we certainly are wasting
> > space. Go back and read the explanation in
> >   http://lists.xen.org/archives/html/xen-devel/2012-12/msg00274.html
>
> I think I probably did not put my point clearly to make it
> understandable. Xen_netbk_rx_ring_full uses max_required_rx_slots
> value. Xen_netbk_rx_ring_full is called to decide wither a vif is
> schedulable or not. So in case the mtu value is >=PAGE_SIZE, for a
> worst case scenario additional buffer would be required which is not
> taken care by current calculations.

Correct, I agree with your description of the current problem.

> Ofcourse in your new fix if you do a code change not to leave a hole
> in first buffer then this correction may not be required. But I am
> not the right person to decide the implications of the fix you are
> proposing. The current start_new_rx_buffer seems to be trying to
> make the copies PAGE aligned and also reduce number of copy
> operations.

Indeed, that is what the change that we're testing now does. Changing
start_new_rx_buffer() to avoid leaving a hole in the first buffer is
performing quite well. Unfortunately I don't have comparison numbers
just now.

> For example let us say SKB_HEAD_LEN is for whatever reason
> 4*PAGE_SIZE and offset_in_page is 32.
>
> As per existing logic of start_new_rx_buffer and with the fix I am
> proposing for count and copy_off, we will calculate and occupy 5
> ring buffers and will use 5 copy operations.

I agree.

> If we fix it the way you are proposing, not to leave a hole in the
> first buffer by modifying start_new_rx_buffer then it will occupy 4
> ring buffers but will require 8 copy operations as per existing
> logic in netbk_gop_skb while copying head!!

I think that we should optimize for more commonly seen lengths, which
I'd think would be 2-3 pages max. The delta in copy operations is
smaller in these cases, where we would use 4 copy operations for 2
pages (as opposed to 3 with the current algorithm) and 6 copy
operations for 3 pages (as opposed to 4 with the current algorithm).

IanC, Konrad - do you have any opinions on the best approach here?

Matt

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel