Re: [Xen-devel] [PATCH RFC] xen-netback: calculate the number of slots required for large MTU vifs

[annie li <annie.li@oracle.com>]

On 2013-7-11 14:01, annie li wrote:
>
> On 2013-7-11 13:14, Matt Wilson wrote:
>> On Wed, Jul 10, 2013 at 08:37:03PM +0100, Wei Liu wrote:
>>> On Wed, Jul 10, 2013 at 09:13:33AM +0100, Wei Liu wrote:
>>>> On Tue, Jul 09, 2013 at 10:40:59PM +0000, Matt Wilson wrote:
>>>>> From: Xi Xiong <xixiong@amazon.com>
>>>>>
>>>>> [ note: I've just cherry picked this onto net-next, and only compile
>>>>>    tested. This a RFC only. -msw ]
>>>>>
>>>> Should probably rebase it on net.git because it is a bug fix. Let's
>>>> worry about that later...
>> *nod*
>>
>>>>> Currently the number of RX slots required to transmit a SKB to
>>>>> xen-netfront can be miscalculated when an interface uses a MTU larger
>>>>> than PAGE_SIZE. If the slot calculation is wrong, xen-netback can
>>>>> pause the queue indefinitely or reuse slots. The former manifests 
>>>>> as a
>>>>> loss of connectivity to the guest (which can be restored by lowering
>>>>> the MTU set on the interface). The latter manifests with "Bad grant
>>>>> reference" messages from Xen such as:
>>>>>
>>>>> (XEN) grant_table.c:1797:d0 Bad grant reference 264241157
>>>>>
>>>>> and kernel messages within the guest such as:
>>>>>
>>>>> [  180.419567] net eth0: Invalid extra type: 112
>>>>> [  180.868620] net eth0: rx->offset: 0, size: 4294967295
>>>>> [  180.868629] net eth0: rx->offset: 0, size: 4294967295
>>>>>
>>>>> BUG_ON() assertions can also be hit if RX slots are exhausted while
>>>>> handling a SKB.
>>>>>
>>>>> This patch changes xen_netbk_rx_action() to count the number of RX
>>>>> slots actually consumed by netbk_gop_skb() instead of using 
>>>>> nr_frags + 1.
>>>>> This prevents under-counting the number of RX slots consumed when a
>>>>> SKB has a large linear buffer.
>>>>>
>>>>> Additionally, we now store the estimated number of RX slots required
>>>>> to handle a SKB in the cb overlay. This value is used to determine if
>>>>> the next SKB in the queue can be processed.
>>>>>
>>>>> Finally, the logic in start_new_rx_buffer() can cause RX slots to be
>>>>> wasted when setting up copy grant table operations for SKBs with 
>>>>> large
>>>>> linear buffers. For example, a SKB with skb_headlen() equal to 8157
>>>>> bytes that starts 64 bytes 64 bytes from the start of the page will
>>>> Duplicated "64 bytes". And this change looks like an improvement not a
>>>> bug fix. Probably submit a separate patch for this?
>> Argh, I knew it was in there somewhere (since you pointed it out in
>> Dubin :-).
>>
>> Maybe it could be a separate patch. I think the description is also a
>> bit confusing. I'll work on rewording it.
>>
>>>>> consume three RX slots instead of two. This patch changes the "head"
>>>>> parameter to netbk_gop_frag_copy() to act as a flag. When set,
>>>>> start_new_rx_buffer() will always place as much data as possible into
>>>>> each RX slot.
>>>>>
>>>>> Signed-off-by: Xi Xiong <xixiong@amazon.com>
>>>>> Reviewed-by: Matt Wilson <msw@amazon.com>
>>>>> [ msw: minor code cleanups, rewrote commit message, adjusted code
>>>>>    to count RX slots instead of meta structures ]
>>>>> Signed-off-by: Matt Wilson <msw@amazon.com>
>>>>> Cc: Annie Li <annie.li@oracle.com>
>>>>> Cc: Wei Liu <wei.liu2@citrix.com>
>>>>> Cc: Ian Campbell <Ian.Campbell@citrix.com>
>>>>> Cc: netdev@vger.kernel.org
>>>>> Cc: xen-devel@lists.xenproject.org
>>>>> ---
>>>>>   drivers/net/xen-netback/netback.c |   51 
>>>>> ++++++++++++++++++++++--------------
>>>>>   1 files changed, 31 insertions(+), 20 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/xen-netback/netback.c 
>>>>> b/drivers/net/xen-netback/netback.c
>>>>> index 64828de..82dd207 100644
>>>>> --- a/drivers/net/xen-netback/netback.c
>>>>> +++ b/drivers/net/xen-netback/netback.c
>>>>> @@ -110,6 +110,11 @@ union page_ext {
>>>>>       void *mapping;
>>>>>   };
>>>>>   +struct skb_cb_overlay {
>>>>> +    int meta_slots_used;
>>>>> +    int peek_slots_count;
>>>>> +};
>>>>> +
>>>>>   struct xen_netbk {
>>>>>       wait_queue_head_t wq;
>>>>>       struct task_struct *task;
>>>>> @@ -370,6 +375,7 @@ unsigned int xen_netbk_count_skb_slots(struct 
>>>>> xenvif *vif, struct sk_buff *skb)
>>>>>   {
>>>>>       unsigned int count;
>>>>>       int i, copy_off;
>>>>> +    struct skb_cb_overlay *sco;
>>>>>         count = DIV_ROUND_UP(skb_headlen(skb), PAGE_SIZE);
>>>>>   @@ -411,6 +417,9 @@ unsigned int 
>>>>> xen_netbk_count_skb_slots(struct xenvif *vif, struct sk_buff *skb)
>>>>>                   offset = 0;
>>>>>           }
>>>>>       }
>>>>> +
>>>>> +    sco = (struct skb_cb_overlay *) skb->cb;
>>>>> +    sco->peek_slots_count = count;
>>>>>       return count;
>>>>>   }
>>>>>   @@ -443,13 +452,12 @@ static struct netbk_rx_meta 
>>>>> *get_next_rx_buffer(struct xenvif *vif,
>>>>>   }
>>>>>     /*
>>>>> - * Set up the grant operations for this fragment. If it's a flipping
>>>>> - * interface, we also set up the unmap request from here.
>>>>> + * Set up the grant operations for this fragment.
>>>>>    */
>>>>>   static void netbk_gop_frag_copy(struct xenvif *vif, struct 
>>>>> sk_buff *skb,
>>>>>                   struct netrx_pending_operations *npo,
>>>>>                   struct page *page, unsigned long size,
>>>>> -                unsigned long offset, int *head)
>>>>> +                unsigned long offset, int head, int *first)
>>>>>   {
>>>>>       struct gnttab_copy *copy_gop;
>>>>>       struct netbk_rx_meta *meta;
>>>>> @@ -479,12 +487,12 @@ static void netbk_gop_frag_copy(struct 
>>>>> xenvif *vif, struct sk_buff *skb,
>>>>>           if (bytes > size)
>>>>>               bytes = size;
>>>>>   -        if (start_new_rx_buffer(npo->copy_off, bytes, *head)) {
>>>>> +        if (start_new_rx_buffer(npo->copy_off, bytes, head)) {
>
> head is always 1 here when processing the header data, so it is 
> impossible to start a new buffer for header data with larger header 
> size, and get_next_rx_buffer is never called. I assume this would not 
> work well for skb with larger header data size. Have you run network 
> test with this patch?

Sorry, I forgot the offset == MAX_BUFFER_OFFSET case and misunderstand 
your patch, please ignore my last comments. Your patch keeps the 
original DIV_ROUND_UP and changes the mechanism in netbk_gop_frag_copy 
to make slots same with xen_netbk_count_skb_slots. All Roads Lead to 
Rome!:-)

Thanks
Annie