Re: [Xen-devel] bogus wrap check in xen-netback

[Wei Liu]

On Wed, Apr 25, 2018 at 12:35:11PM +0200, Olaf Hering wrote:
> Am Wed, 25 Apr 2018 11:31:25 +0100
> schrieb Wei Liu :
> 
> > My bad. Yes, they are converted to int, not unsigned int.
> 
> Hopefully that happens only if the target is int, not if all involved 
> variables are short.
> 
> Unless there are objections I will prepare a patch to deal with RING_IDX 
> being u16.
> 

RING_IDX is a type defined in the public header -- I don't think you can
change that at all. You don't know what third party drivers rely on
that.

If you want to change the type locally in netback, then why? Aren't you
just debugging?

In that light, I don't see a point having a patch to deal with u16.

Wei.


> Olaf



___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] bogus wrap check in xen-netback

[Olaf Hering]

Am Wed, 25 Apr 2018 11:31:25 +0100
schrieb Wei Liu :

> My bad. Yes, they are converted to int, not unsigned int.

Hopefully that happens only if the target is int, not if all involved variables 
are short.

Unless there are objections I will prepare a patch to deal with RING_IDX being 
u16.

Olaf


pgpKZjIyrez3i.pgp
Description: Digitale Signatur von OpenPGP
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] bogus wrap check in xen-netback

[Wei Liu]

On Wed, Apr 25, 2018 at 04:26:06AM -0600, Jan Beulich wrote:
> >>> On 25.04.18 at 12:06,  wrote:
> > On Wed, Apr 25, 2018 at 11:04:26AM +0200, Olaf Hering wrote:
> >> Am Wed, 25 Apr 2018 09:59:23 +0100
> >> schrieb Wei Liu :
> >> 
> >> > Do you have the full diff of your changes? 
> >> 
> >> Not right now. But without 'val', or val being uint, the same error 
> >> happens 
> > in f():
> >> 
> >> #include 
> >> void f(void)
> >> {
> >> unsigned short req_prod = 0, req_cons = 65400;
> >> unsigned short val;
> >> val = req_prod - req_cons;
> >> printf("req_prod - req_cons %u\n", val);
> >> printf("req_prod - req_cons %x\n", val);
> >> }
> > 
> > What Jan said.
> > 
> > Integer promotion makes unsigned short into unsigned int first then do
> > the calculation.
> 
> Not exactly - it promotes to plain (i.e. signed) int.

My bad. Yes, they are converted to int, not unsigned int.

Wei.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] bogus wrap check in xen-netback

[Jan Beulich]

>>> On 25.04.18 at 12:06,  wrote:
> On Wed, Apr 25, 2018 at 11:04:26AM +0200, Olaf Hering wrote:
>> Am Wed, 25 Apr 2018 09:59:23 +0100
>> schrieb Wei Liu :
>> 
>> > Do you have the full diff of your changes? 
>> 
>> Not right now. But without 'val', or val being uint, the same error happens 
> in f():
>> 
>> #include 
>> void f(void)
>> {
>> unsigned short req_prod = 0, req_cons = 65400;
>> unsigned short val;
>> val = req_prod - req_cons;
>> printf("req_prod - req_cons %u\n", val);
>> printf("req_prod - req_cons %x\n", val);
>> }
> 
> What Jan said.
> 
> Integer promotion makes unsigned short into unsigned int first then do
> the calculation.

Not exactly - it promotes to plain (i.e. signed) int.

Jan



___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] bogus wrap check in xen-netback

[Wei Liu]

On Wed, Apr 25, 2018 at 11:04:26AM +0200, Olaf Hering wrote:
> Am Wed, 25 Apr 2018 09:59:23 +0100
> schrieb Wei Liu :
> 
> > Do you have the full diff of your changes? 
> 
> Not right now. But without 'val', or val being uint, the same error happens 
> in f():
> 
> #include 
> void f(void)
> {
> unsigned short req_prod = 0, req_cons = 65400;
> unsigned short val;
> val = req_prod - req_cons;
> printf("req_prod - req_cons %u\n", val);
> printf("req_prod - req_cons %x\n", val);
> }

What Jan said.

Integer promotion makes unsigned short into unsigned int first then do
the calculation. Assigning the result to val truncates it back to
unsigned short.

For the original code, idx is of type unsigned int. No promotion or
truncation is needed so the end result is correct.

Wei.

> 
> int main(void)
> {
> #if 1
> unsigned nr_ents = 0x100U, req_prod_pvt = 0x14U, rsp_cons = 
> 0xffeeU, req_prod = 0xfffeU;
> unsigned rx_target = 0x40U, qlen = 0x1aU;
> #else
> unsigned nr_ents = 0x100U, req_prod_pvt = 0x00U, rsp_cons = 
> 0xffeeU, req_prod = 0xfffeU;
> unsigned rx_target = 0x40U, qlen = 0x1aU;
> #endif
> printf("batch_target %u, skb_queue_len %u, rx_target %u\n", rx_target 
> - (req_prod_pvt - rsp_cons), qlen, rx_target);
> printf("nr_ents %u\n", nr_ents);
> printf("req_prod_pvt - rsp_cons %u\n", req_prod_pvt - rsp_cons);
> printf("req_prod_pvt - req_prod %u\n", req_prod_pvt - req_prod);
> printf("%u\n", nr_ents - (req_prod_pvt - rsp_cons));
> printf("%u\n", nr_ents - (req_prod_pvt - rsp_cons));
> f();
> return 0;
> }



___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] bogus wrap check in xen-netback

[Jan Beulich]

>>> On 25.04.18 at 11:04,  wrote:
> Am Wed, 25 Apr 2018 09:59:23 +0100
> schrieb Wei Liu :
> 
>> Do you have the full diff of your changes? 
> 
> Not right now. But without 'val', or val being uint, the same error happens 
> in f():
> 
> #include 
> void f(void)
> {
> unsigned short req_prod = 0, req_cons = 65400;
> unsigned short val;
> val = req_prod - req_cons;
> printf("req_prod - req_cons %u\n", val);
> printf("req_prod - req_cons %x\n", val);
> }

Well, of course - anything more narrow than int will be promoted to int first.

Jan



___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] bogus wrap check in xen-netback

[Olaf Hering]

Am Wed, 25 Apr 2018 09:59:23 +0100
schrieb Wei Liu :

> Do you have the full diff of your changes? 

Not right now. But without 'val', or val being uint, the same error happens in 
f():

#include 
void f(void)
{
unsigned short req_prod = 0, req_cons = 65400;
unsigned short val;
val = req_prod - req_cons;
printf("req_prod - req_cons %u\n", val);
printf("req_prod - req_cons %x\n", val);
}

int main(void)
{
#if 1
unsigned nr_ents = 0x100U, req_prod_pvt = 0x14U, rsp_cons = 
0xffeeU, req_prod = 0xfffeU;
unsigned rx_target = 0x40U, qlen = 0x1aU;
#else
unsigned nr_ents = 0x100U, req_prod_pvt = 0x00U, rsp_cons = 
0xffeeU, req_prod = 0xfffeU;
unsigned rx_target = 0x40U, qlen = 0x1aU;
#endif
printf("batch_target %u, skb_queue_len %u, rx_target %u\n", rx_target - 
(req_prod_pvt - rsp_cons), qlen, rx_target);
printf("nr_ents %u\n", nr_ents);
printf("req_prod_pvt - rsp_cons %u\n", req_prod_pvt - rsp_cons);
printf("req_prod_pvt - req_prod %u\n", req_prod_pvt - req_prod);
printf("%u\n", nr_ents - (req_prod_pvt - rsp_cons));
printf("%u\n", nr_ents - (req_prod_pvt - rsp_cons));
f();
return 0;
}


pgpUg7lnDX9qJ.pgp
Description: Digitale Signatur von OpenPGP
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] bogus wrap check in xen-netback

[Wei Liu]

On Wed, Apr 25, 2018 at 09:39:24AM +0200, Olaf Hering wrote:
> Am Wed, 25 Apr 2018 09:28:38 +0200
> schrieb Juergen Gross :
> 
> > Why? (u16)0 - (u16)65400 == 136
> 
> My helloworld.c shows that ushort gets promoted to uint, unless it is done 
> like that:
> 
> -   if (queue->tx.sring->req_prod - queue->tx.req_cons >
> -   XEN_NETIF_TX_RING_SIZE) {
> +   idx = queue->tx.sring->req_prod - queue->tx.req_cons; 
> +   if ( idx > XEN_NETIF_TX_RING_SIZE) {

Do you have the full diff of your changes? I don't follow why integer
conversion works differently if you write the code like this. As far as
I can tell the type of LHS didn't change.

Wei.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] bogus wrap check in xen-netback

[Olaf Hering]

Am Wed, 25 Apr 2018 09:28:38 +0200
schrieb Juergen Gross :

> Why? (u16)0 - (u16)65400 == 136

My helloworld.c shows that ushort gets promoted to uint, unless it is done like 
that:

-   if (queue->tx.sring->req_prod - queue->tx.req_cons >
-   XEN_NETIF_TX_RING_SIZE) {
+   idx = queue->tx.sring->req_prod - queue->tx.req_cons; 
+   if ( idx > XEN_NETIF_TX_RING_SIZE) {

Olaf


pgpAzwssvPxQm.pgp
Description: Digitale Signatur von OpenPGP
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] bogus wrap check in xen-netback

[Juergen Gross]

On 25/04/18 09:19, Olaf Hering wrote:
> With commit 48856286b64e ("xen/netback: shutdown the ring if it contains 
> garbage.") a new check was added to xen-netback, which triggers for me.
> 
> Since there are bugs in ring buffer handling I tried to trigger them earlier 
> by changing RING_IDX from u32 to u16. Now I found another one, and I wonder 
> if the error below could potentially also hit with u32:
> 
> ...
> [  624.186492] br0: port 3(vif2.0) entered forwarding state
> [  624.186522] br0: port 3(vif2.0) entered forwarding state
> [  680.865398] vif vif-1-0 vif1.0: Impossible number of requests. req_prod 0, 
> req_cons 65400, size 256
> [  680.865402] vif vif-1-0 vif1.0: fatal error; disabling device
> [  680.865495] br0: port 2(vif1.0) entered disabled state
> [  689.433849] vif vif-2-0 vif2.0: Impossible number of requests. req_prod 0, 
> req_cons 65527, size 256
> [  689.433857] vif vif-2-0 vif2.0: fatal error; disabling device
> [  689.433945] br0: port 3(vif2.0) entered disabled state
> [  690.930512] pktgen: Packet Generator for packet performance testing. 
> Version: 2.75
> ...
> 
> What exactly is that check in xenvif_tx_build_gops trying to achieve? 
> Subtracting a non-zero value from zero will always create something larger 
> than XEN_NETIF_TX_RING_SIZE.

Why? (u16)0 - (u16)65400 == 136


Juergen

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel