* bogus wrap check in xen-netback
@ 2018-04-25 7:19 Olaf Hering
2018-04-25 7:28 ` Juergen Gross
0 siblings, 1 reply; 11+ messages in thread
From: Olaf Hering @ 2018-04-25 7:19 UTC (permalink / raw)
To: xen-devel, Wei Liu, Paul Durrant
[-- Attachment #1.1: Type: text/plain, Size: 1222 bytes --]
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.
Olaf
[-- Attachment #1.2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: bogus wrap check in xen-netback 2018-04-25 7:19 bogus wrap check in xen-netback Olaf Hering @ 2018-04-25 7:28 ` Juergen Gross 2018-04-25 7:39 ` Olaf Hering 0 siblings, 1 reply; 11+ messages in thread From: Juergen Gross @ 2018-04-25 7:28 UTC (permalink / raw) To: Olaf Hering, xen-devel, Wei Liu, Paul Durrant 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: bogus wrap check in xen-netback 2018-04-25 7:28 ` Juergen Gross @ 2018-04-25 7:39 ` Olaf Hering 2018-04-25 8:59 ` Wei Liu 0 siblings, 1 reply; 11+ messages in thread From: Olaf Hering @ 2018-04-25 7:39 UTC (permalink / raw) To: Juergen Gross; +Cc: xen-devel, Paul Durrant, Wei Liu [-- Attachment #1.1: Type: text/plain, Size: 457 bytes --] Am Wed, 25 Apr 2018 09:28:38 +0200 schrieb Juergen Gross <jgross@suse.com>: > 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 [-- Attachment #1.2: Digitale Signatur von OpenPGP --] [-- Type: application/pgp-signature, Size: 195 bytes --] [-- Attachment #2: Type: text/plain, Size: 157 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: bogus wrap check in xen-netback 2018-04-25 7:39 ` Olaf Hering @ 2018-04-25 8:59 ` Wei Liu 2018-04-25 9:04 ` Olaf Hering 0 siblings, 1 reply; 11+ messages in thread From: Wei Liu @ 2018-04-25 8:59 UTC (permalink / raw) To: Olaf Hering; +Cc: Juergen Gross, xen-devel, Paul Durrant, 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 <jgross@suse.com>: > > > 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: bogus wrap check in xen-netback 2018-04-25 8:59 ` Wei Liu @ 2018-04-25 9:04 ` Olaf Hering 2018-04-25 9:48 ` Jan Beulich 2018-04-25 10:06 ` Wei Liu 0 siblings, 2 replies; 11+ messages in thread From: Olaf Hering @ 2018-04-25 9:04 UTC (permalink / raw) To: Wei Liu; +Cc: Juergen Gross, xen-devel, Paul Durrant [-- Attachment #1.1: Type: text/plain, Size: 1308 bytes --] Am Wed, 25 Apr 2018 09:59:23 +0100 schrieb Wei Liu <wei.liu2@citrix.com>: > 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 <stdio.h> 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 = 0xffffffeeU, req_prod = 0xfffffffeU; unsigned rx_target = 0x40U, qlen = 0x1aU; #else unsigned nr_ents = 0x100U, req_prod_pvt = 0x00U, rsp_cons = 0xffffffeeU, req_prod = 0xfffffffeU; 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; } [-- Attachment #1.2: Digitale Signatur von OpenPGP --] [-- Type: application/pgp-signature, Size: 195 bytes --] [-- Attachment #2: Type: text/plain, Size: 157 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: bogus wrap check in xen-netback 2018-04-25 9:04 ` Olaf Hering @ 2018-04-25 9:48 ` Jan Beulich 2018-04-25 10:06 ` Wei Liu 1 sibling, 0 replies; 11+ messages in thread From: Jan Beulich @ 2018-04-25 9:48 UTC (permalink / raw) To: Olaf Hering, Wei Liu; +Cc: Juergen Gross, xen-devel, Paul Durrant >>> On 25.04.18 at 11:04, <olaf@aepfle.de> wrote: > Am Wed, 25 Apr 2018 09:59:23 +0100 > schrieb Wei Liu <wei.liu2@citrix.com>: > >> 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 <stdio.h> > 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: bogus wrap check in xen-netback 2018-04-25 9:04 ` Olaf Hering 2018-04-25 9:48 ` Jan Beulich @ 2018-04-25 10:06 ` Wei Liu 2018-04-25 10:26 ` Jan Beulich 1 sibling, 1 reply; 11+ messages in thread From: Wei Liu @ 2018-04-25 10:06 UTC (permalink / raw) To: Olaf Hering; +Cc: Juergen Gross, xen-devel, Paul Durrant, 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 <wei.liu2@citrix.com>: > > > 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 <stdio.h> > 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 = 0xffffffeeU, req_prod = 0xfffffffeU; > unsigned rx_target = 0x40U, qlen = 0x1aU; > #else > unsigned nr_ents = 0x100U, req_prod_pvt = 0x00U, rsp_cons = 0xffffffeeU, req_prod = 0xfffffffeU; > 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: bogus wrap check in xen-netback 2018-04-25 10:06 ` Wei Liu @ 2018-04-25 10:26 ` Jan Beulich 2018-04-25 10:31 ` Wei Liu 0 siblings, 1 reply; 11+ messages in thread From: Jan Beulich @ 2018-04-25 10:26 UTC (permalink / raw) To: Olaf Hering, Wei Liu; +Cc: Juergen Gross, xen-devel, Paul Durrant >>> On 25.04.18 at 12:06, <wei.liu2@citrix.com> 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 <wei.liu2@citrix.com>: >> >> > 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 <stdio.h> >> 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: bogus wrap check in xen-netback 2018-04-25 10:26 ` Jan Beulich @ 2018-04-25 10:31 ` Wei Liu 2018-04-25 10:35 ` Olaf Hering 0 siblings, 1 reply; 11+ messages in thread From: Wei Liu @ 2018-04-25 10:31 UTC (permalink / raw) To: Jan Beulich; +Cc: Juergen Gross, xen-devel, Olaf Hering, Paul Durrant, Wei Liu On Wed, Apr 25, 2018 at 04:26:06AM -0600, Jan Beulich wrote: > >>> On 25.04.18 at 12:06, <wei.liu2@citrix.com> 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 <wei.liu2@citrix.com>: > >> > >> > 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 <stdio.h> > >> 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: bogus wrap check in xen-netback 2018-04-25 10:31 ` Wei Liu @ 2018-04-25 10:35 ` Olaf Hering 2018-04-25 10:47 ` Wei Liu 0 siblings, 1 reply; 11+ messages in thread From: Olaf Hering @ 2018-04-25 10:35 UTC (permalink / raw) To: Wei Liu; +Cc: Juergen Gross, xen-devel, Paul Durrant, Jan Beulich [-- Attachment #1.1: Type: text/plain, Size: 328 bytes --] Am Wed, 25 Apr 2018 11:31:25 +0100 schrieb Wei Liu <wei.liu2@citrix.com>: > 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 [-- Attachment #1.2: Digitale Signatur von OpenPGP --] [-- Type: application/pgp-signature, Size: 195 bytes --] [-- Attachment #2: Type: text/plain, Size: 157 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: bogus wrap check in xen-netback 2018-04-25 10:35 ` Olaf Hering @ 2018-04-25 10:47 ` Wei Liu 0 siblings, 0 replies; 11+ messages in thread From: Wei Liu @ 2018-04-25 10:47 UTC (permalink / raw) To: Olaf Hering; +Cc: Juergen Gross, xen-devel, Paul Durrant, Wei Liu, Jan Beulich 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 <wei.liu2@citrix.com>: > > > 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-04-25 10:47 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-04-25 7:19 bogus wrap check in xen-netback Olaf Hering 2018-04-25 7:28 ` Juergen Gross 2018-04-25 7:39 ` Olaf Hering 2018-04-25 8:59 ` Wei Liu 2018-04-25 9:04 ` Olaf Hering 2018-04-25 9:48 ` Jan Beulich 2018-04-25 10:06 ` Wei Liu 2018-04-25 10:26 ` Jan Beulich 2018-04-25 10:31 ` Wei Liu 2018-04-25 10:35 ` Olaf Hering 2018-04-25 10:47 ` Wei Liu
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.