Re: bug disabling guest interface

Re: bug disabling guest interface

[Steven Haigh]

[-- Attachment #1.1: Type: text/plain, Size: 581 bytes --]

Hi all,

I've been following this with some interest - as I've been getting 
numerous reports of this happening recently.

As Jacek Milewicz posted recently:
http://lists.xen.org/archives/html/xen-devel/2013-03/msg00404.html

It seems to affect both linux and windows DomUs.

Is there any further information on a possible fix for this? Thoughts? 
Ideas? As it seems to be hitting multiple sites, I'd like to get it 
fixed asap...

-- 
Steven Haigh

Email: netwiz@crc.id.au
Web: https://www.crc.id.au
Phone: (03) 9001 6090 - 0412 935 897
Fax: (03) 8338 0299


[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4240 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: bug disabling guest interface

[Jacek Milewicz]

Hi All

> 
> Is there any further information on a possible fix for this? Thoughts?
> Ideas? As it seems to be hitting multiple sites, I'd like to get it
fixed asap...

There are 3 ways I can see this fixed:
- update guests so all have same MAX_SKB_FRAGS (that includes windows
drivers (windows drivers use 19 for MAX_SKB_FRAGS))
-add some sort of negotiation between host and guest
- change MAX_SKB_FRAGS to 19 to accommodate all guests

Unfortunately first one requires changes to the guest and most don't have
that luxury. So the only way I see it could be fixed without breaking
compatibility even more  is

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 821c7f4..82de0f5 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -143,8 +143,8 @@ struct sk_buff;
  * Since GRO uses frags we allocate at least 16 regardless of page
  * size.
  */
-#if (65536/PAGE_SIZE + 1) < 16
-#define MAX_SKB_FRAGS 16UL
+#if (65536/PAGE_SIZE + 1) < 19
+#define MAX_SKB_FRAGS 19UL
 #else
 #define MAX_SKB_FRAGS (65536/PAGE_SIZE + 1)
 #endif

Is: SKB_MAX_LEN bites again. Was: Re: bug disabling guest interface

[Konrad Rzeszutek Wilk]

On Fri, Mar 08, 2013 at 08:36:37PM +0100, Jacek Milewicz wrote:
> Hi All
> 
> > 
> > Is there any further information on a possible fix for this? Thoughts?
> > Ideas? As it seems to be hitting multiple sites, I'd like to get it
> fixed asap...
> 
> There are 3 ways I can see this fixed:
> - update guests so all have same MAX_SKB_FRAGS (that includes windows
> drivers (windows drivers use 19 for MAX_SKB_FRAGS))
> -add some sort of negotiation between host and guest
> - change MAX_SKB_FRAGS to 19 to accommodate all guests
> 
> Unfortunately first one requires changes to the guest and most don't have
> that luxury. So the only way I see it could be fixed without breaking
> compatibility even more  is

Ugh. The negotiations between host and guest is probably the best
choice. The issues you are going to hit are that you might need
to redo the skbs to match what the frontend's max is.

Annie, Wei, Ian - were there some RFC patches floating around
for this?

> 
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 821c7f4..82de0f5 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -143,8 +143,8 @@ struct sk_buff;
>   * Since GRO uses frags we allocate at least 16 regardless of page
>   * size.
>   */
> -#if (65536/PAGE_SIZE + 1) < 16
> -#define MAX_SKB_FRAGS 16UL
> +#if (65536/PAGE_SIZE + 1) < 19
> +#define MAX_SKB_FRAGS 19UL
>  #else
>  #define MAX_SKB_FRAGS (65536/PAGE_SIZE + 1)
>  #endif
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

Re: Is: SKB_MAX_LEN bites again. Was: Re: bug disabling guest interface

[Jacek Milewicz]

> > > Is there any further information on a possible fix for this?
Thoughts?
> > > Ideas? As it seems to be hitting multiple sites, I'd like to get it
> > fixed asap...
> >
> > There are 3 ways I can see this fixed:
> > - update guests so all have same MAX_SKB_FRAGS (that includes windows
> > drivers (windows drivers use 19 for MAX_SKB_FRAGS)) -add some sort of
> > negotiation between host and guest
> > - change MAX_SKB_FRAGS to 19 to accommodate all guests
> >
> > Unfortunately first one requires changes to the guest and most don't
> > have that luxury. So the only way I see it could be fixed without
> > breaking compatibility even more  is
> 
> Ugh. The negotiations between host and guest is probably the best
choice.
> The issues you are going to hit are that you might need to redo the skbs
to
> match what the frontend's max is.
> 
> Annie, Wei, Ian - were there some RFC patches floating around for this?
> 

As much as I agree that negotiation is the best option in long term. It
relies on people upgrading their kernels - and this is not the case (only
people running kernels from before 2011-12-23 are affected by this bug
(I'm not sure about other platforms)).
And in a lot of cases (VPS providers) there is no easy way to upgrade
guest kernels. 

Re: Is: SKB_MAX_LEN bites again. Was: Re: bug disabling guest interface

[Steven Haigh]

[-- Attachment #1.1: Type: text/plain, Size: 1663 bytes --]

On 9/03/2013 9:09 AM, Jacek Milewicz wrote:
>>>> Is there any further information on a possible fix for this?
> Thoughts?
>>>> Ideas? As it seems to be hitting multiple sites, I'd like to get it
>>> fixed asap...
>>>
>>> There are 3 ways I can see this fixed:
>>> - update guests so all have same MAX_SKB_FRAGS (that includes windows
>>> drivers (windows drivers use 19 for MAX_SKB_FRAGS)) -add some sort of
>>> negotiation between host and guest
>>> - change MAX_SKB_FRAGS to 19 to accommodate all guests
>>>
>>> Unfortunately first one requires changes to the guest and most don't
>>> have that luxury. So the only way I see it could be fixed without
>>> breaking compatibility even more  is
>>
>> Ugh. The negotiations between host and guest is probably the best
> choice.
>> The issues you are going to hit are that you might need to redo the skbs
> to
>> match what the frontend's max is.
>>
>> Annie, Wei, Ian - were there some RFC patches floating around for this?
>>
>
> As much as I agree that negotiation is the best option in long term. It
> relies on people upgrading their kernels - and this is not the case (only
> people running kernels from before 2011-12-23 are affected by this bug
> (I'm not sure about other platforms)).
> And in a lot of cases (VPS providers) there is no easy way to upgrade
> guest kernels.

For what its worth, I've increased the value of MAX_SKB_FRAGS to 19 on 
my Dom0 kernel and pushed out new packages... I'll see if I get any 
further reports...

-- 
Steven Haigh

Email: netwiz@crc.id.au
Web: https://www.crc.id.au
Phone: (03) 9001 6090 - 0412 935 897
Fax: (03) 8338 0299


[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4240 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: Is: SKB_MAX_LEN bites again. Was: Re: bug disabling guest interface

[Ian Campbell]

> > - change MAX_SKB_FRAGS to 19 to accommodate all guests

Changing MAX_SKB_FRAGS is *not* an option upstream. This might be a
useful local hack but we need to drop the idea as a long term fix.

> Ugh. The negotiations between host and guest is probably the best
> choice. The issues you are going to hit are that you might need
> to redo the skbs to match what the frontend's max is.

IMHO the right fix is for netback to coalesce as it copies from the
frontend if it needs to do so, it is copying anyway so it should be
cheap enough. I thought we had discussed this and someone was working on
implementing it. If not Annie then perhaps it was Matt or Siva (both now
CC'd)

If necessary netback could even allocate a larger order head in order to
accommodate very large packets, but I don't expect that to be required
to fix the immediate issue we are seeing (but gives flexibility)

This should get us past the immediate issue of the upstream change from
18->16  frags thing. Longer term the negotiation will allow us to avoid
future incompatible changes in guest and host network stacks, as well as
allowing frontends on other OSes (in particular Windows) to havea better
chance of to DTRT.
 
> Annie, Wei, Ian - were there some RFC patches floating around
> for this?
> 
> > 
> > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> > index 821c7f4..82de0f5 100644
> > --- a/include/linux/skbuff.h
> > +++ b/include/linux/skbuff.h
> > @@ -143,8 +143,8 @@ struct sk_buff;
> >   * Since GRO uses frags we allocate at least 16 regardless of page
> >   * size.
> >   */
> > -#if (65536/PAGE_SIZE + 1) < 16
> > -#define MAX_SKB_FRAGS 16UL
> > +#if (65536/PAGE_SIZE + 1) < 19
> > +#define MAX_SKB_FRAGS 19UL
> >  #else
> >  #define MAX_SKB_FRAGS (65536/PAGE_SIZE + 1)
> >  #endif
> > 
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
> > 

Re: Is: SKB_MAX_LEN bites again. Was: Re: bug disabling guest interface

[Steven Haigh]

[-- Attachment #1.1: Type: text/plain, Size: 2416 bytes --]

On 9/03/2013 1:57 PM, Ian Campbell wrote:
>>> - change MAX_SKB_FRAGS to 19 to accommodate all guests
>
> Changing MAX_SKB_FRAGS is *not* an option upstream. This might be a
> useful local hack but we need to drop the idea as a long term fix.

I agree. Its a hack that seems to work until we have something else to 
offer though. I'll be honest, the internals of the kernel interactions 
like this is a bit beyond my knowledge - but I get quite a few emails of 
stuff going strange. This one has increased dramatically in the past week.

>> Ugh. The negotiations between host and guest is probably the best
>> choice. The issues you are going to hit are that you might need
>> to redo the skbs to match what the frontend's max is.
>
> IMHO the right fix is for netback to coalesce as it copies from the
> frontend if it needs to do so, it is copying anyway so it should be
> cheap enough. I thought we had discussed this and someone was working on
> implementing it. If not Annie then perhaps it was Matt or Siva (both now
> CC'd)

I did see some talk about it on the xen-devel lists quite some time ago 
- however it seemed to die with no outcome that I could find. Maybe its 
a good time for a nudge on the matter :)

> If necessary netback could even allocate a larger order head in order to
> accommodate very large packets, but I don't expect that to be required
> to fix the immediate issue we are seeing (but gives flexibility)
>
> This should get us past the immediate issue of the upstream change from
> 18->16  frags thing. Longer term the negotiation will allow us to avoid
> future incompatible changes in guest and host network stacks, as well as
> allowing frontends on other OSes (in particular Windows) to havea better
> chance of to DTRT.

Whatever the fix, it has to be on the Dom0 kernel - as stated by others 
in the past, it isn't a feasible fix to include changes on the client. 
There would be too many different guest OSes that would make 
implementation a nightmare - or in fact impossible.

>> Annie, Wei, Ian - were there some RFC patches floating around
>> for this?

I didn't stumble across any in the list archives that I hunted for. Not 
to say they don't exist, but I didn't find them if they do exist.

-- 
Steven Haigh

Email: netwiz@crc.id.au
Web: https://www.crc.id.au
Phone: (03) 9001 6090 - 0412 935 897
Fax: (03) 8338 0299


[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4240 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: Is: SKB_MAX_LEN bites again. Was: Re: bug disabling guest interface

[Sander Eikelenboom]

Saturday, March 9, 2013, 3:57:16 AM, you wrote:

>> > - change MAX_SKB_FRAGS to 19 to accommodate all guests

> Changing MAX_SKB_FRAGS is *not* an option upstream. This might be a
> useful local hack but we need to drop the idea as a long term fix.

>> Ugh. The negotiations between host and guest is probably the best
>> choice. The issues you are going to hit are that you might need
>> to redo the skbs to match what the frontend's max is.

> IMHO the right fix is for netback to coalesce as it copies from the
> frontend if it needs to do so, it is copying anyway so it should be
> cheap enough. I thought we had discussed this and someone was working on
> implementing it. If not Annie then perhaps it was Matt or Siva (both now
> CC'd)

> If necessary netback could even allocate a larger order head in order to
> accommodate very large packets, but I don't expect that to be required
> to fix the immediate issue we are seeing (but gives flexibility)

> This should get us past the immediate issue of the upstream change from
18->>16  frags thing. Longer term the negotiation will allow us to avoid
> future incompatible changes in guest and host network stacks, as well as
> allowing frontends on other OSes (in particular Windows) to havea better
> chance of to DTRT.
>  
>> Annie, Wei, Ian - were there some RFC patches floating around
>> for this?

I think you refer to this thread ?

http://lists.xen.org/archives/html/xen-devel/2013-01/msg00198.html

And i see the annie's patch and my response haven't made the list because annie dropped xen-devel from the response.
I will forward them ..

--
Sander





>> 
>> > 
>> > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>> > index 821c7f4..82de0f5 100644
>> > --- a/include/linux/skbuff.h
>> > +++ b/include/linux/skbuff.h
>> > @@ -143,8 +143,8 @@ struct sk_buff;
>> >   * Since GRO uses frags we allocate at least 16 regardless of page
>> >   * size.
>> >   */
>> > -#if (65536/PAGE_SIZE + 1) < 16
>> > -#define MAX_SKB_FRAGS 16UL
>> > +#if (65536/PAGE_SIZE + 1) < 19
>> > +#define MAX_SKB_FRAGS 19UL
>> >  #else
>> >  #define MAX_SKB_FRAGS (65536/PAGE_SIZE + 1)
>> >  #endif
>> > 
>> > 
>> > _______________________________________________
>> > Xen-devel mailing list
>> > Xen-devel@lists.xen.org
>> > http://lists.xen.org/xen-devel
>> > 

Re: Is: SKB_MAX_LEN bites again. Was: Re: bug disabling guest interface

[ANNIE LI]

On 2013-3-9 4:36, Konrad Rzeszutek Wilk wrote:
> On Fri, Mar 08, 2013 at 08:36:37PM +0100, Jacek Milewicz wrote:
>> Hi All
>>
>>> Is there any further information on a possible fix for this? Thoughts?
>>> Ideas? As it seems to be hitting multiple sites, I'd like to get it
>> fixed asap...
>>
>> There are 3 ways I can see this fixed:
>> - update guests so all have same MAX_SKB_FRAGS (that includes windows
>> drivers (windows drivers use 19 for MAX_SKB_FRAGS))
>> -add some sort of negotiation between host and guest
>> - change MAX_SKB_FRAGS to 19 to accommodate all guests
>>
>> Unfortunately first one requires changes to the guest and most don't have
>> that luxury. So the only way I see it could be fixed without breaking
>> compatibility even more  is
> Ugh. The negotiations between host and guest is probably the best
> choice. The issues you are going to hit are that you might need
> to redo the skbs to match what the frontend's max is.
>
> Annie, Wei, Ian - were there some RFC patches floating around
> for this?

Actually I did plan to do it, but failed to start it because of windows 
WHQL things.

Thanks
Annie
>
>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>> index 821c7f4..82de0f5 100644
>> --- a/include/linux/skbuff.h
>> +++ b/include/linux/skbuff.h
>> @@ -143,8 +143,8 @@ struct sk_buff;
>>    * Since GRO uses frags we allocate at least 16 regardless of page
>>    * size.
>>    */
>> -#if (65536/PAGE_SIZE + 1)<  16
>> -#define MAX_SKB_FRAGS 16UL
>> +#if (65536/PAGE_SIZE + 1)<  19
>> +#define MAX_SKB_FRAGS 19UL
>>   #else
>>   #define MAX_SKB_FRAGS (65536/PAGE_SIZE + 1)
>>   #endif
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>>

Re: Is: SKB_MAX_LEN bites again. Was: Re: bug disabling guest interface

[ANNIE LI]

On 2013-3-9 10:57, Ian Campbell wrote:
>>> - change MAX_SKB_FRAGS to 19 to accommodate all guests
> Changing MAX_SKB_FRAGS is *not* an option upstream. This might be a
> useful local hack but we need to drop the idea as a long term fix.
>
>> Ugh. The negotiations between host and guest is probably the best
>> choice. The issues you are going to hit are that you might need
>> to redo the skbs to match what the frontend's max is.
> IMHO the right fix is for netback to coalesce as it copies from the
> frontend if it needs to do so, it is copying anyway so it should be
> cheap enough. I thought we had discussed this and someone was working on
> implementing it. If not Annie then perhaps it was Matt or Siva (both now
> CC'd)

Sorry, I have been working on some windows whql thing these days, so did 
not start it till now.
I did do some patch for netfront before(Sander mentioned later), which 
did some segment for large skbs in netfront. This is different from what 
you mentioned. I assume we need to do some work in both netback and 
netfront.

Thanks
Annie

>
> If necessary netback could even allocate a larger order head in order to
> accommodate very large packets, but I don't expect that to be required
> to fix the immediate issue we are seeing (but gives flexibility)
>
> This should get us past the immediate issue of the upstream change from
> 18->16  frags thing. Longer term the negotiation will allow us to avoid
> future incompatible changes in guest and host network stacks, as well as
> allowing frontends on other OSes (in particular Windows) to havea better
> chance of to DTRT.
>
>> Annie, Wei, Ian - were there some RFC patches floating around
>> for this?
>>
>>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>>> index 821c7f4..82de0f5 100644
>>> --- a/include/linux/skbuff.h
>>> +++ b/include/linux/skbuff.h
>>> @@ -143,8 +143,8 @@ struct sk_buff;
>>>    * Since GRO uses frags we allocate at least 16 regardless of page
>>>    * size.
>>>    */
>>> -#if (65536/PAGE_SIZE + 1)<  16
>>> -#define MAX_SKB_FRAGS 16UL
>>> +#if (65536/PAGE_SIZE + 1)<  19
>>> +#define MAX_SKB_FRAGS 19UL
>>>   #else
>>>   #define MAX_SKB_FRAGS (65536/PAGE_SIZE + 1)
>>>   #endif
>>>
>>>
>>> _______________________________________________
>>> Xen-devel mailing list
>>> Xen-devel@lists.xen.org
>>> http://lists.xen.org/xen-devel
>>>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: Is: SKB_MAX_LEN bites again. Was: Re: bug disabling guest interface

[ANNIE LI]

On 2013-3-9 20:53, Sander Eikelenboom wrote:
> Saturday, March 9, 2013, 3:57:16 AM, you wrote:
>
>>>> - change MAX_SKB_FRAGS to 19 to accommodate all guests
>> Changing MAX_SKB_FRAGS is *not* an option upstream. This might be a
>> useful local hack but we need to drop the idea as a long term fix.
>>> Ugh. The negotiations between host and guest is probably the best
>>> choice. The issues you are going to hit are that you might need
>>> to redo the skbs to match what the frontend's max is.
>> IMHO the right fix is for netback to coalesce as it copies from the
>> frontend if it needs to do so, it is copying anyway so it should be
>> cheap enough. I thought we had discussed this and someone was working on
>> implementing it. If not Annie then perhaps it was Matt or Siva (both now
>> CC'd)
>> If necessary netback could even allocate a larger order head in order to
>> accommodate very large packets, but I don't expect that to be required
>> to fix the immediate issue we are seeing (but gives flexibility)
>> This should get us past the immediate issue of the upstream change from
> 18->>16  frags thing. Longer term the negotiation will allow us to avoid
>> future incompatible changes in guest and host network stacks, as well as
>> allowing frontends on other OSes (in particular Windows) to havea better
>> chance of to DTRT.
>>
>>> Annie, Wei, Ian - were there some RFC patches floating around
>>> for this?
> I think you refer to this thread ?
>
> http://lists.xen.org/archives/html/xen-devel/2013-01/msg00198.html
>
> And i see the annie's patch and my response haven't made the list because annie dropped xen-devel from the response.
> I will forward them ..

Hi Sander,

This is a patch in which I tried to do some fix in netfront, the thread 
you mentioned is facing the same issue with this one.

Thanks
Annie
>
> --
> Sander
>
>
>
>
>
>>>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>>>> index 821c7f4..82de0f5 100644
>>>> --- a/include/linux/skbuff.h
>>>> +++ b/include/linux/skbuff.h
>>>> @@ -143,8 +143,8 @@ struct sk_buff;
>>>>    * Since GRO uses frags we allocate at least 16 regardless of page
>>>>    * size.
>>>>    */
>>>> -#if (65536/PAGE_SIZE + 1)<  16
>>>> -#define MAX_SKB_FRAGS 16UL
>>>> +#if (65536/PAGE_SIZE + 1)<  19
>>>> +#define MAX_SKB_FRAGS 19UL
>>>>   #else
>>>>   #define MAX_SKB_FRAGS (65536/PAGE_SIZE + 1)
>>>>   #endif
>>>>
>>>>
>>>> _______________________________________________
>>>> Xen-devel mailing list
>>>> Xen-devel@lists.xen.org
>>>> http://lists.xen.org/xen-devel
>>>>
>
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: Is: SKB_MAX_LEN bites again. Was: Re: bug disabling guest interface

[Wei Liu]

On Sat, Mar 09, 2013 at 02:57:16AM +0000, Ian Campbell wrote:
> > > - change MAX_SKB_FRAGS to 19 to accommodate all guests
> 
> Changing MAX_SKB_FRAGS is *not* an option upstream. This might be a
> useful local hack but we need to drop the idea as a long term fix.
> 
> > Ugh. The negotiations between host and guest is probably the best
> > choice. The issues you are going to hit are that you might need
> > to redo the skbs to match what the frontend's max is.
> 
> IMHO the right fix is for netback to coalesce as it copies from the
> frontend if it needs to do so, it is copying anyway so it should be
> cheap enough. I thought we had discussed this and someone was working on
> implementing it. If not Annie then perhaps it was Matt or Siva (both now
> CC'd)
> 

As a short term fix, can we use skb_linearize() if skb->nr_frags >=
MAX_SKB_FRAGS?


Wei.

Re: Is: SKB_MAX_LEN bites again. Was: Re: bug disabling guest interface

[Ian Campbell]

On Sun, 2013-03-10 at 04:49 +0000, ANNIE LI wrote:
> I did do some patch for netfront before(Sander mentioned later), which 
> did some segment for large skbs in netfront. This is different from what 
> you mentioned. I assume we need to do some work in both netback and 
> netfront.

The fix I'm talking about in
<1362797836.8941.189.camel@hastur.hellion.org.uk> should be netback
only.

Ian.

Re: Is: SKB_MAX_LEN bites again. Was: Re: bug disabling guest interface

[Ian Campbell]

On Sun, 2013-03-10 at 19:18 +0000, Wei Liu wrote:
> On Sat, Mar 09, 2013 at 02:57:16AM +0000, Ian Campbell wrote:
> > > > - change MAX_SKB_FRAGS to 19 to accommodate all guests
> > 
> > Changing MAX_SKB_FRAGS is *not* an option upstream. This might be a
> > useful local hack but we need to drop the idea as a long term fix.
> > 
> > > Ugh. The negotiations between host and guest is probably the best
> > > choice. The issues you are going to hit are that you might need
> > > to redo the skbs to match what the frontend's max is.
> > 
> > IMHO the right fix is for netback to coalesce as it copies from the
> > frontend if it needs to do so, it is copying anyway so it should be
> > cheap enough. I thought we had discussed this and someone was working on
> > implementing it. If not Annie then perhaps it was Matt or Siva (both now
> > CC'd)
> > 
> 
> As a short term fix, can we use skb_linearize() if skb->nr_frags >=
> MAX_SKB_FRAGS?

No, because that would require changing the frontend, while this fix
needs to be in the backend if older guests are to continue working.

You can't use skb_linearize in netback as is because you would first
need to be able to build the skb with nr_frags >= MAX_SKB_FRAGS in order
to pass it to that function.

I'd worry about failing the higher order allocations needed to linearize
in this way too -- better to coalesce as much as possible into single
page frags.

Ian.

Re: Is: SKB_MAX_LEN bites again. Was: Re: bug disabling guest interface

[Wei Liu]

On Tue, 2013-03-12 at 11:40 +0000, Ian Campbell wrote:
> On Sun, 2013-03-10 at 19:18 +0000, Wei Liu wrote:
> > On Sat, Mar 09, 2013 at 02:57:16AM +0000, Ian Campbell wrote:
> > > > > - change MAX_SKB_FRAGS to 19 to accommodate all guests
> > > 
> > > Changing MAX_SKB_FRAGS is *not* an option upstream. This might be a
> > > useful local hack but we need to drop the idea as a long term fix.
> > > 
> > > > Ugh. The negotiations between host and guest is probably the best
> > > > choice. The issues you are going to hit are that you might need
> > > > to redo the skbs to match what the frontend's max is.
> > > 
> > > IMHO the right fix is for netback to coalesce as it copies from the
> > > frontend if it needs to do so, it is copying anyway so it should be
> > > cheap enough. I thought we had discussed this and someone was working on
> > > implementing it. If not Annie then perhaps it was Matt or Siva (both now
> > > CC'd)
> > > 
> > 
> > As a short term fix, can we use skb_linearize() if skb->nr_frags >=
> > MAX_SKB_FRAGS?
> 
> No, because that would require changing the frontend, while this fix
> needs to be in the backend if older guests are to continue working.
> 
> You can't use skb_linearize in netback as is because you would first
> need to be able to build the skb with nr_frags >= MAX_SKB_FRAGS in order
> to pass it to that function.
> 

Yes, the idea is to define NETBK_MAX_SKB_FRAGS to some bigger number
(say 20) to accommodate the possible maximum number of frags in
frontend. The thing that truly matters it the skb->len, which should be
<64K, nr_frags is not important.

I wrote some code during weekend and it seemed to work, but I have not
verify it extensively.


Wei.

Re: Is: SKB_MAX_LEN bites again. Was: Re: bug disabling guest interface

[Konrad Rzeszutek Wilk]

On Fri, Mar 08, 2013 at 11:09:56PM +0100, Jacek Milewicz wrote:
> > > > Is there any further information on a possible fix for this?
> Thoughts?
> > > > Ideas? As it seems to be hitting multiple sites, I'd like to get it
> > > fixed asap...
> > >
> > > There are 3 ways I can see this fixed:
> > > - update guests so all have same MAX_SKB_FRAGS (that includes windows
> > > drivers (windows drivers use 19 for MAX_SKB_FRAGS)) -add some sort of
> > > negotiation between host and guest
> > > - change MAX_SKB_FRAGS to 19 to accommodate all guests
> > >
> > > Unfortunately first one requires changes to the guest and most don't
> > > have that luxury. So the only way I see it could be fixed without
> > > breaking compatibility even more  is
> > 
> > Ugh. The negotiations between host and guest is probably the best
> choice.
> > The issues you are going to hit are that you might need to redo the skbs
> to
> > match what the frontend's max is.
> > 
> > Annie, Wei, Ian - were there some RFC patches floating around for this?
> > 
> 
> As much as I agree that negotiation is the best option in long term. It
> relies on people upgrading their kernels - and this is not the case (only
> people running kernels from before 2011-12-23 are affected by this bug
> (I'm not sure about other platforms)).
> And in a lot of cases (VPS providers) there is no easy way to upgrade
> guest kernels. 

Sure. The other option in the xen-netback might be a configurable option
to let net-back know that it is running with older guests which expect
18 size MAX_SKB_FRAGS. That way we have the negotiation part, the code to
deal with backend MAX_SKB_FRAGS != frontend MAX_SKB_FRAGS and the value that
the system admin provides.

What I am saying is that if we get the negotiation protcol figured out
properly we can provide a per-guest value that the system admin
can over-write to say: "I know that this guest uses _this_ size".

Re: Is: SKB_MAX_LEN bites again. Was: Re: bug disabling guest interface

[Konrad Rzeszutek Wilk]

On Tue, Mar 12, 2013 at 10:48:07AM -0400, Konrad Rzeszutek Wilk wrote:
> On Fri, Mar 08, 2013 at 11:09:56PM +0100, Jacek Milewicz wrote:
> > > > > Is there any further information on a possible fix for this?
> > Thoughts?
> > > > > Ideas? As it seems to be hitting multiple sites, I'd like to get it
> > > > fixed asap...
> > > >
> > > > There are 3 ways I can see this fixed:
> > > > - update guests so all have same MAX_SKB_FRAGS (that includes windows
> > > > drivers (windows drivers use 19 for MAX_SKB_FRAGS)) -add some sort of
> > > > negotiation between host and guest
> > > > - change MAX_SKB_FRAGS to 19 to accommodate all guests
> > > >
> > > > Unfortunately first one requires changes to the guest and most don't
> > > > have that luxury. So the only way I see it could be fixed without
> > > > breaking compatibility even more  is
> > > 
> > > Ugh. The negotiations between host and guest is probably the best
> > choice.
> > > The issues you are going to hit are that you might need to redo the skbs
> > to
> > > match what the frontend's max is.
> > > 
> > > Annie, Wei, Ian - were there some RFC patches floating around for this?
> > > 
> > 
> > As much as I agree that negotiation is the best option in long term. It
> > relies on people upgrading their kernels - and this is not the case (only
> > people running kernels from before 2011-12-23 are affected by this bug
> > (I'm not sure about other platforms)).
> > And in a lot of cases (VPS providers) there is no easy way to upgrade
> > guest kernels. 
> 
> Sure. But the other option in the xen-netback might be a configurable option
> to let net-back know that it is running with older guests which expect
> 18 size MAX_SKB_FRAGS. That way we have the negotiation part, the code to
> deal with backend MAX_SKB_FRAGS != frontend MAX_SKB_FRAGS and the value that
> the system admin provides.
> 

Re-sending as I think it never made it out.

Re: Is: SKB_MAX_LEN bites again. Was: Re: bug disabling guest interface

[Ian Campbell]

On Tue, 2013-03-12 at 14:49 +0000, Konrad Rzeszutek Wilk wrote:
> On Fri, Mar 08, 2013 at 11:09:56PM +0100, Jacek Milewicz wrote:
> > > > > Is there any further information on a possible fix for this?
> > Thoughts?
> > > > > Ideas? As it seems to be hitting multiple sites, I'd like to get it
> > > > fixed asap...
> > > >
> > > > There are 3 ways I can see this fixed:
> > > > - update guests so all have same MAX_SKB_FRAGS (that includes windows
> > > > drivers (windows drivers use 19 for MAX_SKB_FRAGS)) -add some sort of
> > > > negotiation between host and guest
> > > > - change MAX_SKB_FRAGS to 19 to accommodate all guests
> > > >
> > > > Unfortunately first one requires changes to the guest and most don't
> > > > have that luxury. So the only way I see it could be fixed without
> > > > breaking compatibility even more  is
> > > 
> > > Ugh. The negotiations between host and guest is probably the best
> > choice.
> > > The issues you are going to hit are that you might need to redo the skbs
> > to
> > > match what the frontend's max is.
> > > 
> > > Annie, Wei, Ian - were there some RFC patches floating around for this?
> > > 
> > 
> > As much as I agree that negotiation is the best option in long term. It
> > relies on people upgrading their kernels - and this is not the case (only
> > people running kernels from before 2011-12-23 are affected by this bug
> > (I'm not sure about other platforms)).
> > And in a lot of cases (VPS providers) there is no easy way to upgrade
> > guest kernels. 
> 
> Sure. The other option in the xen-netback might be a configurable option
> to let net-back know that it is running with older guests which expect
> 18 size MAX_SKB_FRAGS. That way we have the negotiation part, the code to
> deal with backend MAX_SKB_FRAGS != frontend MAX_SKB_FRAGS and the value that
> the system admin provides.

I think this is over complicating things. netback can and should be
modified to be able to cope with guests using frags == 18 (or larger)
even if it only itself supports frags == 16 by coalescing things in the
obvious way. This works for all guest supplied frames up to 64K in size,
regardless of the number of slots they use to represent that 64K.

The negotiation stuff is pretty orthogonal to this and is more about the
MTU of GSO frames. In terms of priority this is a distant second to the
above.

Ian.

Re: Is: SKB_MAX_LEN bites again. Was: Re: bug disabling guest interface

[Steven Haigh]

On 03/13/2013 01:56 AM, Ian Campbell wrote:
> On Tue, 2013-03-12 at 14:49 +0000, Konrad Rzeszutek Wilk wrote:
>> On Fri, Mar 08, 2013 at 11:09:56PM +0100, Jacek Milewicz wrote:
>>>>>> Is there any further information on a possible fix for this?
>>> Thoughts?
>>>>>> Ideas? As it seems to be hitting multiple sites, I'd like to get it
>>>>> fixed asap...
>>>>>
>>>>> There are 3 ways I can see this fixed:
>>>>> - update guests so all have same MAX_SKB_FRAGS (that includes windows
>>>>> drivers (windows drivers use 19 for MAX_SKB_FRAGS)) -add some sort of
>>>>> negotiation between host and guest
>>>>> - change MAX_SKB_FRAGS to 19 to accommodate all guests
>>>>>
>>>>> Unfortunately first one requires changes to the guest and most don't
>>>>> have that luxury. So the only way I see it could be fixed without
>>>>> breaking compatibility even more  is
>>>>
>>>> Ugh. The negotiations between host and guest is probably the best
>>> choice.
>>>> The issues you are going to hit are that you might need to redo the skbs
>>> to
>>>> match what the frontend's max is.
>>>>
>>>> Annie, Wei, Ian - were there some RFC patches floating around for this?
>>>>
>>>
>>> As much as I agree that negotiation is the best option in long term. It
>>> relies on people upgrading their kernels - and this is not the case (only
>>> people running kernels from before 2011-12-23 are affected by this bug
>>> (I'm not sure about other platforms)).
>>> And in a lot of cases (VPS providers) there is no easy way to upgrade
>>> guest kernels.
>>
>> Sure. The other option in the xen-netback might be a configurable option
>> to let net-back know that it is running with older guests which expect
>> 18 size MAX_SKB_FRAGS. That way we have the negotiation part, the code to
>> deal with backend MAX_SKB_FRAGS != frontend MAX_SKB_FRAGS and the value that
>> the system admin provides.
>
> I think this is over complicating things. netback can and should be
> modified to be able to cope with guests using frags == 18 (or larger)
> even if it only itself supports frags == 16 by coalescing things in the
> obvious way. This works for all guest supplied frames up to 64K in size,
> regardless of the number of slots they use to represent that 64K.

On this, while we just hacked this in the kernel source to increase 
MAX_SKB_FRAGS to 19, I believe Jacek Milewicz tested 18 to work fine on 
linux guests, but it seems Windows guests would still fail unless it was 
increased to 19.

-- 
Steven Haigh

Email: netwiz@crc.id.au
Web: https://www.crc.id.au
Phone: (03) 9001 6090 - 0412 935 897
Fax: (03) 8338 0299

Re: Is: SKB_MAX_LEN bites again. Was: Re: bug disabling guest interface

[ANNIE LI]

On 2013-3-12 20:18, Wei Liu wrote:
> On Tue, 2013-03-12 at 11:40 +0000, Ian Campbell wrote:
>> On Sun, 2013-03-10 at 19:18 +0000, Wei Liu wrote:
>>> On Sat, Mar 09, 2013 at 02:57:16AM +0000, Ian Campbell wrote:
>>>>>> - change MAX_SKB_FRAGS to 19 to accommodate all guests
>>>> Changing MAX_SKB_FRAGS is *not* an option upstream. This might be a
>>>> useful local hack but we need to drop the idea as a long term fix.
>>>>
>>>>> Ugh. The negotiations between host and guest is probably the best
>>>>> choice. The issues you are going to hit are that you might need
>>>>> to redo the skbs to match what the frontend's max is.
>>>> IMHO the right fix is for netback to coalesce as it copies from the
>>>> frontend if it needs to do so, it is copying anyway so it should be
>>>> cheap enough. I thought we had discussed this and someone was working on
>>>> implementing it. If not Annie then perhaps it was Matt or Siva (both now
>>>> CC'd)
>>>>
>>> As a short term fix, can we use skb_linearize() if skb->nr_frags>=
>>> MAX_SKB_FRAGS?
>> No, because that would require changing the frontend, while this fix
>> needs to be in the backend if older guests are to continue working.
>>
>> You can't use skb_linearize in netback as is because you would first
>> need to be able to build the skb with nr_frags>= MAX_SKB_FRAGS in order
>> to pass it to that function.
>>
> Yes, the idea is to define NETBK_MAX_SKB_FRAGS to some bigger number
> (say 20) to accommodate the possible maximum number of frags in
> frontend. The thing that truly matters it the skb->len, which should be
> <64K, nr_frags is not important.

I doubt this would work since you can not build out skb with nr_frags >= 
MAX_SKB_FRAGS. See following code in skbuff.h,

#if (65536/PAGE_SIZE + 1) < 16
#define MAX_SKB_FRAGS 16UL
#else
#define MAX_SKB_FRAGS (65536/PAGE_SIZE + 1)
#endif

and every skb contains MAX_SKB_FRAGS frags,

struct skb_shared_info {
....
skb_frag_t      frags[MAX_SKB_FRAGS];
...
}

Coalescing frags before building skb could avoid this issue.

Thanks
Annie
>
> I wrote some code during weekend and it seemed to work, but I have not
> verify it extensively.
>
>
> Wei.
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: Is: SKB_MAX_LEN bites again. Was: Re: bug disabling guest interface

[Ian Campbell]

On Tue, 2013-03-12 at 15:05 +0000, Steven Haigh wrote:
> On 03/13/2013 01:56 AM, Ian Campbell wrote:
> > On Tue, 2013-03-12 at 14:49 +0000, Konrad Rzeszutek Wilk wrote:
> >> On Fri, Mar 08, 2013 at 11:09:56PM +0100, Jacek Milewicz wrote:
> >>>>>> Is there any further information on a possible fix for this?
> >>> Thoughts?
> >>>>>> Ideas? As it seems to be hitting multiple sites, I'd like to get it
> >>>>> fixed asap...
> >>>>>
> >>>>> There are 3 ways I can see this fixed:
> >>>>> - update guests so all have same MAX_SKB_FRAGS (that includes windows
> >>>>> drivers (windows drivers use 19 for MAX_SKB_FRAGS)) -add some sort of
> >>>>> negotiation between host and guest
> >>>>> - change MAX_SKB_FRAGS to 19 to accommodate all guests
> >>>>>
> >>>>> Unfortunately first one requires changes to the guest and most don't
> >>>>> have that luxury. So the only way I see it could be fixed without
> >>>>> breaking compatibility even more  is
> >>>>
> >>>> Ugh. The negotiations between host and guest is probably the best
> >>> choice.
> >>>> The issues you are going to hit are that you might need to redo the skbs
> >>> to
> >>>> match what the frontend's max is.
> >>>>
> >>>> Annie, Wei, Ian - were there some RFC patches floating around for this?
> >>>>
> >>>
> >>> As much as I agree that negotiation is the best option in long term. It
> >>> relies on people upgrading their kernels - and this is not the case (only
> >>> people running kernels from before 2011-12-23 are affected by this bug
> >>> (I'm not sure about other platforms)).
> >>> And in a lot of cases (VPS providers) there is no easy way to upgrade
> >>> guest kernels.
> >>
> >> Sure. The other option in the xen-netback might be a configurable option
> >> to let net-back know that it is running with older guests which expect
> >> 18 size MAX_SKB_FRAGS. That way we have the negotiation part, the code to
> >> deal with backend MAX_SKB_FRAGS != frontend MAX_SKB_FRAGS and the value that
> >> the system admin provides.
> >
> > I think this is over complicating things. netback can and should be
> > modified to be able to cope with guests using frags == 18 (or larger)
> > even if it only itself supports frags == 16 by coalescing things in the
> > obvious way. This works for all guest supplied frames up to 64K in size,
> > regardless of the number of slots they use to represent that 64K.
> 
> On this, while we just hacked this in the kernel source to increase 
> MAX_SKB_FRAGS to 19, I believe Jacek Milewicz tested 18 to work fine on 
> linux guests, but it seems Windows guests would still fail unless it was 
> increased to 19.

No netback has TTBOMK ever support 19, so I don't know how those guests
ever worked, but regardless doing coalescing in the backend would fix
this at least for frames which are <64K in size.

Ian.

Re: Is: SKB_MAX_LEN bites again. Was: Re: bug disabling guest interface

[Wei Liu]

On Tue, 2013-03-12 at 15:07 +0000, ANNIE LI wrote:
> 
> On 2013-3-12 20:18, Wei Liu wrote:
> > On Tue, 2013-03-12 at 11:40 +0000, Ian Campbell wrote:
> >> On Sun, 2013-03-10 at 19:18 +0000, Wei Liu wrote:
> >>> On Sat, Mar 09, 2013 at 02:57:16AM +0000, Ian Campbell wrote:
> >>>>>> - change MAX_SKB_FRAGS to 19 to accommodate all guests
> >>>> Changing MAX_SKB_FRAGS is *not* an option upstream. This might be a
> >>>> useful local hack but we need to drop the idea as a long term fix.
> >>>>
> >>>>> Ugh. The negotiations between host and guest is probably the best
> >>>>> choice. The issues you are going to hit are that you might need
> >>>>> to redo the skbs to match what the frontend's max is.
> >>>> IMHO the right fix is for netback to coalesce as it copies from the
> >>>> frontend if it needs to do so, it is copying anyway so it should be
> >>>> cheap enough. I thought we had discussed this and someone was working on
> >>>> implementing it. If not Annie then perhaps it was Matt or Siva (both now
> >>>> CC'd)
> >>>>
> >>> As a short term fix, can we use skb_linearize() if skb->nr_frags>=
> >>> MAX_SKB_FRAGS?
> >> No, because that would require changing the frontend, while this fix
> >> needs to be in the backend if older guests are to continue working.
> >>
> >> You can't use skb_linearize in netback as is because you would first
> >> need to be able to build the skb with nr_frags>= MAX_SKB_FRAGS in order
> >> to pass it to that function.
> >>
> > Yes, the idea is to define NETBK_MAX_SKB_FRAGS to some bigger number
> > (say 20) to accommodate the possible maximum number of frags in
> > frontend. The thing that truly matters it the skb->len, which should be
> > <64K, nr_frags is not important.
> 
> I doubt this would work since you can not build out skb with nr_frags >= 
> MAX_SKB_FRAGS. See following code in skbuff.h,
> 
> #if (65536/PAGE_SIZE + 1) < 16
> #define MAX_SKB_FRAGS 16UL
> #else
> #define MAX_SKB_FRAGS (65536/PAGE_SIZE + 1)
> #endif
> 
> and every skb contains MAX_SKB_FRAGS frags,
> 
> struct skb_shared_info {
> ....
> skb_frag_t      frags[MAX_SKB_FRAGS];
> ...
> }
> 

So I haven't gone this far to trigger this. :-(

I just double checked with printk, nr_frags has not exceeded backend
MAX_SBK_FRAGS even if I have frontend MAX_SKB_FRAGS = 25.

> Coalescing frags before building skb could avoid this issue.
> 

"before building skb" is a bit vague to me. Is it netbk_count_request?
Is it xen_netbk_fill_frags? If it is the latter, we are already too late
to fix this. If it is the former, we don't even start copying.


Wei

Re: Is: SKB_MAX_LEN bites again. Was: Re: bug disabling guest interface

[Wei Liu]

On Tue, 2013-03-12 at 15:25 +0000, Wei Liu wrote:
> On Tue, 2013-03-12 at 15:07 +0000, ANNIE LI wrote:
> > 
> > On 2013-3-12 20:18, Wei Liu wrote:
> > > On Tue, 2013-03-12 at 11:40 +0000, Ian Campbell wrote:
> > >> On Sun, 2013-03-10 at 19:18 +0000, Wei Liu wrote:
> > >>> On Sat, Mar 09, 2013 at 02:57:16AM +0000, Ian Campbell wrote:
> > >>>>>> - change MAX_SKB_FRAGS to 19 to accommodate all guests
> > >>>> Changing MAX_SKB_FRAGS is *not* an option upstream. This might be a
> > >>>> useful local hack but we need to drop the idea as a long term fix.
> > >>>>
> > >>>>> Ugh. The negotiations between host and guest is probably the best
> > >>>>> choice. The issues you are going to hit are that you might need
> > >>>>> to redo the skbs to match what the frontend's max is.
> > >>>> IMHO the right fix is for netback to coalesce as it copies from the
> > >>>> frontend if it needs to do so, it is copying anyway so it should be
> > >>>> cheap enough. I thought we had discussed this and someone was working on
> > >>>> implementing it. If not Annie then perhaps it was Matt or Siva (both now
> > >>>> CC'd)
> > >>>>
> > >>> As a short term fix, can we use skb_linearize() if skb->nr_frags>=
> > >>> MAX_SKB_FRAGS?
> > >> No, because that would require changing the frontend, while this fix
> > >> needs to be in the backend if older guests are to continue working.
> > >>
> > >> You can't use skb_linearize in netback as is because you would first
> > >> need to be able to build the skb with nr_frags>= MAX_SKB_FRAGS in order
> > >> to pass it to that function.
> > >>
> > > Yes, the idea is to define NETBK_MAX_SKB_FRAGS to some bigger number
> > > (say 20) to accommodate the possible maximum number of frags in
> > > frontend. The thing that truly matters it the skb->len, which should be
> > > <64K, nr_frags is not important.
> > 
> > I doubt this would work since you can not build out skb with nr_frags >= 
> > MAX_SKB_FRAGS. See following code in skbuff.h,
> > 
> > #if (65536/PAGE_SIZE + 1) < 16
> > #define MAX_SKB_FRAGS 16UL
> > #else
> > #define MAX_SKB_FRAGS (65536/PAGE_SIZE + 1)
> > #endif
> > 
> > and every skb contains MAX_SKB_FRAGS frags,
> > 
> > struct skb_shared_info {
> > ....
> > skb_frag_t      frags[MAX_SKB_FRAGS];
> > ...
> > }
> > 
> 
> So I haven't gone this far to trigger this. :-(
> 
> I just double checked with printk, nr_frags has not exceeded backend
> MAX_SBK_FRAGS even if I have frontend MAX_SKB_FRAGS = 25.
> 
> > Coalescing frags before building skb could avoid this issue.
> > 
> 
> "before building skb" is a bit vague to me. Is it netbk_count_request?
> Is it xen_netbk_fill_frags? If it is the latter, we are already too late
> to fix this. If it is the former, we don't even start copying.
> 
> 

Actually the copy is done with Hypervisor, I don't know how it is
possible to do coalesce while copying.

FWIW I came up with an idea. Netback maintains a ring of skb_frag_t
groups. Each group has NETBK_SKB_MAX_FRAGS frags. So overall the size of
this ring is MAX_PENGING_REQS * NETBK_SKB_MAX_FRAGS *
sizeof(skb_frag_t). If NETBK_SKB_MAX_FRAGS == 20, skb_frag_t size == 16,
this ring is 80K large.

If we detect frontend's frags > backend's SKB_MAX_FRAGS, we switch to
use the new ring, and set skb->nr_frags to some specific number (say -1)
and has frag[0].page.p set to ring index. Then we copy data as usual.
Later in before xen_netbk_fill_frags if we detect this skb is should be
constructed via previous skb frag ring, we accommodate those frags to
that skb.

if (skb should be constructed via frag ring)
    construct_skb()
else /* normal path
    xen_netbk_fill_frags

This idea is essentially adding a slow path though, because
construct_skb() potentially needs to copy / move data around.


Wei.

Re: Is: SKB_MAX_LEN bites again. Was: Re: bug disabling guest interface

[Wei Liu]

On Tue, Mar 12, 2013 at 08:13:39PM +0000, Wei Liu wrote:
> 
> Actually the copy is done with Hypervisor, I don't know how it is
> possible to do coalesce while copying.
> 
> FWIW I came up with an idea. Netback maintains a ring of skb_frag_t
> groups. Each group has NETBK_SKB_MAX_FRAGS frags. So overall the size of
> this ring is MAX_PENGING_REQS * NETBK_SKB_MAX_FRAGS *
> sizeof(skb_frag_t). If NETBK_SKB_MAX_FRAGS == 20, skb_frag_t size == 16,
> this ring is 80K large.

Just realized two things: 1. the ring size estimation is wrong; 2. we
don't need a extra ring for this. :-)

We can alter how frags are handled inside netback to solve this issue.
We can store start of pending_idx and end of pending_idx in
skb->frags[0] and skb->frags[1] if frontend's MAX_SKB_FRAGS is larger
than backend's.


Wei.

> 
> If we detect frontend's frags > backend's SKB_MAX_FRAGS, we switch to
> use the new ring, and set skb->nr_frags to some specific number (say -1)
> and has frag[0].page.p set to ring index. Then we copy data as usual.
> Later in before xen_netbk_fill_frags if we detect this skb is should be
> constructed via previous skb frag ring, we accommodate those frags to
> that skb.
> 
> if (skb should be constructed via frag ring)
>     construct_skb()
> else /* normal path
>     xen_netbk_fill_frags
> 
> This idea is essentially adding a slow path though, because
> construct_skb() potentially needs to copy / move data around.
> 
> 
> Wei.
> 

Re: Is: SKB_MAX_LEN bites again. Was: Re: bug disabling guest interface

[James Harper]

> >
> > On this, while we just hacked this in the kernel source to increase
> > MAX_SKB_FRAGS to 19, I believe Jacek Milewicz tested 18 to work fine on
> > linux guests, but it seems Windows guests would still fail unless it was
> > increased to 19.
> 
> No netback has TTBOMK ever support 19, so I don't know how those guests
> ever worked, but regardless doing coalescing in the backend would fix
> this at least for frames which are <64K in size.
> 

Well Windows could easily generate a packet with more than 19 buffers, and I have engineered situations where it could be much larger than that, but you aren't going to see such packets during regular usage.

Is there any consensus yet on what form max_buffers negotiation would take? Is it as simple as the frontend and backend each writing the maximum value they can support (without further work) to their respective xenstores?

What backends exist these days? Are BSD and Solaris backends still in existence? What are their respective limitations?

James

Re: Is: SKB_MAX_LEN bites again. Was: Re: bug disabling guest interface

[jacek burghardt]

[-- Attachment #1.1: Type: text/plain, Size: 141 bytes --]

This seems to create network performance issues. It seems that asterisk
under unstable xen and 3.8.2 kernel gives lots of unreachable errors

[-- Attachment #1.2: Type: text/html, Size: 145 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: Is: SKB_MAX_LEN bites again. Was: Re: bug disabling guest interface

[annie li]

On 2013-3-12 23:25, Wei Liu wrote:
> On Tue, 2013-03-12 at 15:07 +0000, ANNIE LI wrote:
>> On 2013-3-12 20:18, Wei Liu wrote:
>>> On Tue, 2013-03-12 at 11:40 +0000, Ian Campbell wrote:
>>>> On Sun, 2013-03-10 at 19:18 +0000, Wei Liu wrote:
>>>>> On Sat, Mar 09, 2013 at 02:57:16AM +0000, Ian Campbell wrote:
>>>>>>>> - change MAX_SKB_FRAGS to 19 to accommodate all guests
>>>>>> Changing MAX_SKB_FRAGS is *not* an option upstream. This might be a
>>>>>> useful local hack but we need to drop the idea as a long term fix.
>>>>>>
>>>>>>> Ugh. The negotiations between host and guest is probably the best
>>>>>>> choice. The issues you are going to hit are that you might need
>>>>>>> to redo the skbs to match what the frontend's max is.
>>>>>> IMHO the right fix is for netback to coalesce as it copies from the
>>>>>> frontend if it needs to do so, it is copying anyway so it should be
>>>>>> cheap enough. I thought we had discussed this and someone was working on
>>>>>> implementing it. If not Annie then perhaps it was Matt or Siva (both now
>>>>>> CC'd)
>>>>>>
>>>>> As a short term fix, can we use skb_linearize() if skb->nr_frags>=
>>>>> MAX_SKB_FRAGS?
>>>> No, because that would require changing the frontend, while this fix
>>>> needs to be in the backend if older guests are to continue working.
>>>>
>>>> You can't use skb_linearize in netback as is because you would first
>>>> need to be able to build the skb with nr_frags>= MAX_SKB_FRAGS in order
>>>> to pass it to that function.
>>>>
>>> Yes, the idea is to define NETBK_MAX_SKB_FRAGS to some bigger number
>>> (say 20) to accommodate the possible maximum number of frags in
>>> frontend. The thing that truly matters it the skb->len, which should be
>>> <64K, nr_frags is not important.
>> I doubt this would work since you can not build out skb with nr_frags >=
>> MAX_SKB_FRAGS. See following code in skbuff.h,
>>
>> #if (65536/PAGE_SIZE + 1) < 16
>> #define MAX_SKB_FRAGS 16UL
>> #else
>> #define MAX_SKB_FRAGS (65536/PAGE_SIZE + 1)
>> #endif
>>
>> and every skb contains MAX_SKB_FRAGS frags,
>>
>> struct skb_shared_info {
>> ....
>> skb_frag_t      frags[MAX_SKB_FRAGS];
>> ...
>> }
>>
> So I haven't gone this far to trigger this. :-(
>
> I just double checked with printk, nr_frags has not exceeded backend
> MAX_SBK_FRAGS even if I have frontend MAX_SKB_FRAGS = 25.

You mean you changed MAX_SKB_FRAGS to 25 in skbuff.h? Did your frontend 
send out skbs with 25 frags?

>
>> Coalescing frags before building skb could avoid this issue.
>>
> "before building skb" is a bit vague to me. Is it netbk_count_request?
> Is it xen_netbk_fill_frags? If it is the latter, we are already too late
> to fix this. If it is the former, we don't even start copying.

I meant coalescing when doing grant copy after netbk_count_request and 
before xen_netbk_fill_frags. But we should take care of data offset of 
per request and merge them into pages.

Thanks
Annie
>
>
> Wei
>

Re: Is: SKB_MAX_LEN bites again. Was: Re: bug disabling guest interface

[annie li]

On 2013-3-13 4:13, Wei Liu wrote:
> Actually the copy is done with Hypervisor, I don't know how it is
> possible to do coalesce while copying.
>
My understanding is we do coalescing in netback during grant copy. For 
example,

                  gop->source.u.ref = txreq.gref;
                  gop->source.domid = vif->domid;
                  gop->source.offset = txreq.offset;

                  gop->dest.u.gmfn = virt_to_mfn(page_address(page));  
--  here should be the page where we coalesce data into, it is likely 
the page is the one which previous request used.
                  gop->dest.domid = DOMID_SELF;
                  gop->dest.offset = txreq.offset;  -- dest.offset 
should be changed to something which represents the offset where we 
start coalescing data of this request

                  gop->len = txreq.size;  -- len should be the total len 
of data which we coalesce into a page
                  gop->flags = GNTCOPY_source_gref;

With this coalescing, it is possible that we could decrease the total 
frags number of skb in netback.

Thanks
Annie

Re: Is: SKB_MAX_LEN bites again. Was: Re: bug disabling guest interface

[Ian Campbell]

On Tue, 2013-03-12 at 20:13 +0000, Wei Liu wrote:
> On Tue, 2013-03-12 at 15:25 +0000, Wei Liu wrote:
> > On Tue, 2013-03-12 at 15:07 +0000, ANNIE LI wrote:
> > > 
> > > On 2013-3-12 20:18, Wei Liu wrote:
> > > > On Tue, 2013-03-12 at 11:40 +0000, Ian Campbell wrote:
> > > >> On Sun, 2013-03-10 at 19:18 +0000, Wei Liu wrote:
> > > >>> On Sat, Mar 09, 2013 at 02:57:16AM +0000, Ian Campbell wrote:
> > > >>>>>> - change MAX_SKB_FRAGS to 19 to accommodate all guests
> > > >>>> Changing MAX_SKB_FRAGS is *not* an option upstream. This might be a
> > > >>>> useful local hack but we need to drop the idea as a long term fix.
> > > >>>>
> > > >>>>> Ugh. The negotiations between host and guest is probably the best
> > > >>>>> choice. The issues you are going to hit are that you might need
> > > >>>>> to redo the skbs to match what the frontend's max is.
> > > >>>> IMHO the right fix is for netback to coalesce as it copies from the
> > > >>>> frontend if it needs to do so, it is copying anyway so it should be
> > > >>>> cheap enough. I thought we had discussed this and someone was working on
> > > >>>> implementing it. If not Annie then perhaps it was Matt or Siva (both now
> > > >>>> CC'd)
> > > >>>>
> > > >>> As a short term fix, can we use skb_linearize() if skb->nr_frags>=
> > > >>> MAX_SKB_FRAGS?
> > > >> No, because that would require changing the frontend, while this fix
> > > >> needs to be in the backend if older guests are to continue working.
> > > >>
> > > >> You can't use skb_linearize in netback as is because you would first
> > > >> need to be able to build the skb with nr_frags>= MAX_SKB_FRAGS in order
> > > >> to pass it to that function.
> > > >>
> > > > Yes, the idea is to define NETBK_MAX_SKB_FRAGS to some bigger number
> > > > (say 20) to accommodate the possible maximum number of frags in
> > > > frontend. The thing that truly matters it the skb->len, which should be
> > > > <64K, nr_frags is not important.
> > > 
> > > I doubt this would work since you can not build out skb with nr_frags >= 
> > > MAX_SKB_FRAGS. See following code in skbuff.h,
> > > 
> > > #if (65536/PAGE_SIZE + 1) < 16
> > > #define MAX_SKB_FRAGS 16UL
> > > #else
> > > #define MAX_SKB_FRAGS (65536/PAGE_SIZE + 1)
> > > #endif
> > > 
> > > and every skb contains MAX_SKB_FRAGS frags,
> > > 
> > > struct skb_shared_info {
> > > ....
> > > skb_frag_t      frags[MAX_SKB_FRAGS];
> > > ...
> > > }
> > > 
> > 
> > So I haven't gone this far to trigger this. :-(
> > 
> > I just double checked with printk, nr_frags has not exceeded backend
> > MAX_SBK_FRAGS even if I have frontend MAX_SKB_FRAGS = 25.
> > 
> > > Coalescing frags before building skb could avoid this issue.
> > > 
> > 
> > "before building skb" is a bit vague to me. Is it netbk_count_request?
> > Is it xen_netbk_fill_frags? If it is the latter, we are already too late
> > to fix this. If it is the former, we don't even start copying.
> > 
> > 
> 
> Actually the copy is done with Hypervisor, I don't know how it is
> possible to do coalesce while copying.

This can be done trivially by making the appropriate gnttab_copy
hypercalls to copy from the guest memory into dom0 buffers, coalescing
as you go. IOW you split each ring slot request into multiple (batched)
copies in order to fill buffers etc.

> FWIW I came up with an idea. Netback maintains a ring of skb_frag_t
> groups. Each group has NETBK_SKB_MAX_FRAGS frags. So overall the size of
> this ring is MAX_PENGING_REQS * NETBK_SKB_MAX_FRAGS *
> sizeof(skb_frag_t). If NETBK_SKB_MAX_FRAGS == 20, skb_frag_t size == 16,
> this ring is 80K large.
> 
> If we detect frontend's frags > backend's SKB_MAX_FRAGS, we switch to
> use the new ring, and set skb->nr_frags to some specific number (say -1)
> and has frag[0].page.p set to ring index. Then we copy data as usual.
> Later in before xen_netbk_fill_frags if we detect this skb is should be
> constructed via previous skb frag ring, we accommodate those frags to
> that skb.
> 
> if (skb should be constructed via frag ring)
>     construct_skb()
> else /* normal path
>     xen_netbk_fill_frags
> 
> This idea is essentially adding a slow path though, because
> construct_skb() potentially needs to copy / move data around.

This is insanely complex for the problem at hand IMHO.

Ian.

Re: Is: SKB_MAX_LEN bites again. Was: Re: bug disabling guest interface

[Wei Liu]

On Wed, 2013-03-13 at 09:43 +0000, Ian Campbell wrote:
> > 
> > This idea is essentially adding a slow path though, because
> > construct_skb() potentially needs to copy / move data around.
> 
> This is insanely complex for the problem at hand IMHO.
> 

Hah, I guess my brain has the ability to overcomplicate things. :-)


Wei.

> Ian.
> 

Re: Is: SKB_MAX_LEN bites again. Was: Re: bug disabling guest interface

[Wei Liu]

On Wed, 2013-03-13 at 06:22 +0000, annie li wrote:
> On 2013-3-12 23:25, Wei Liu wrote:
> > On Tue, 2013-03-12 at 15:07 +0000, ANNIE LI wrote:
> >> On 2013-3-12 20:18, Wei Liu wrote:
> >>> On Tue, 2013-03-12 at 11:40 +0000, Ian Campbell wrote:
> >>>> On Sun, 2013-03-10 at 19:18 +0000, Wei Liu wrote:
> >>>>> On Sat, Mar 09, 2013 at 02:57:16AM +0000, Ian Campbell wrote:
> >>>>>>>> - change MAX_SKB_FRAGS to 19 to accommodate all guests
> >>>>>> Changing MAX_SKB_FRAGS is *not* an option upstream. This might be a
> >>>>>> useful local hack but we need to drop the idea as a long term fix.
> >>>>>>
> >>>>>>> Ugh. The negotiations between host and guest is probably the best
> >>>>>>> choice. The issues you are going to hit are that you might need
> >>>>>>> to redo the skbs to match what the frontend's max is.
> >>>>>> IMHO the right fix is for netback to coalesce as it copies from the
> >>>>>> frontend if it needs to do so, it is copying anyway so it should be
> >>>>>> cheap enough. I thought we had discussed this and someone was working on
> >>>>>> implementing it. If not Annie then perhaps it was Matt or Siva (both now
> >>>>>> CC'd)
> >>>>>>
> >>>>> As a short term fix, can we use skb_linearize() if skb->nr_frags>=
> >>>>> MAX_SKB_FRAGS?
> >>>> No, because that would require changing the frontend, while this fix
> >>>> needs to be in the backend if older guests are to continue working.
> >>>>
> >>>> You can't use skb_linearize in netback as is because you would first
> >>>> need to be able to build the skb with nr_frags>= MAX_SKB_FRAGS in order
> >>>> to pass it to that function.
> >>>>
> >>> Yes, the idea is to define NETBK_MAX_SKB_FRAGS to some bigger number
> >>> (say 20) to accommodate the possible maximum number of frags in
> >>> frontend. The thing that truly matters it the skb->len, which should be
> >>> <64K, nr_frags is not important.
> >> I doubt this would work since you can not build out skb with nr_frags >=
> >> MAX_SKB_FRAGS. See following code in skbuff.h,
> >>
> >> #if (65536/PAGE_SIZE + 1) < 16
> >> #define MAX_SKB_FRAGS 16UL
> >> #else
> >> #define MAX_SKB_FRAGS (65536/PAGE_SIZE + 1)
> >> #endif
> >>
> >> and every skb contains MAX_SKB_FRAGS frags,
> >>
> >> struct skb_shared_info {
> >> ....
> >> skb_frag_t      frags[MAX_SKB_FRAGS];
> >> ...
> >> }
> >>
> > So I haven't gone this far to trigger this. :-(
> >
> > I just double checked with printk, nr_frags has not exceeded backend
> > MAX_SBK_FRAGS even if I have frontend MAX_SKB_FRAGS = 25.
> 
> You mean you changed MAX_SKB_FRAGS to 25 in skbuff.h? Did your frontend 

Yes. In frontend's kernel.

> send out skbs with 25 frags?
> 

No. The maximum number I see is 17.

I didn't spend much time on this though. It's just a little experiment
during my weekend.


Wei.

Is: SKB_MAX_LEN bites again. Was: Re: bug disabling guest interface

[Brian Menges]

Seems I'm completely bitten by this as well. I did notice one difference though between our old production kernel, and the new one that I'm trying to run (Linux 3.8.6)

Looking into sources for 3.2.14-gogrid, I see:
struct sk_buff;

/* To allow 64K frame to be packed as single skb without frag_list. Since
* GRO uses frags we allocate at least 16 regardless of page size.
*/
#if (65536/PAGE_SIZE + 2) < 16
#define MAX_SKB_FRAGS 16UL
#else
#define MAX_SKB_FRAGS (65536/PAGE_SIZE + 2)
#endif

Looking into sources for 3.8.6-gogrid I see:
struct sk_buff;

/* To allow 64K frame to be packed as single skb without frag_list we
* require 64K/PAGE_SIZE pages plus 1 additional page to allow for
* buffers which do not start on a page boundary.
*
* Since GRO uses frags we allocate at least 16 regardless of page
* size.
*/
#if (65536/PAGE_SIZE + 1) < 16
#define MAX_SKB_FRAGS 16UL
#else
#define MAX_SKB_FRAGS (65536/PAGE_SIZE + 1)
#endif

I'm supposing that we could change both instances of '+1' to '+2' and see if this temporarily patches us until netback (???) is patched up.

- Brian Menges
Principal Engineer, DevOps
GoGrid | ServePath | ColoServe | UpStream Networks

The information contained in this message, and any attachments, may contain confidential and legally privileged material. It is solely for the use of the person or entity to which it is addressed.  Any review, retransmission, dissemination, or action taken in reliance upon this information by persons or entities other than the intended recipient is prohibited. If you receive this in error, please contact the sender and delete the material from any computer.