Re: remove function pointer casts and constify function tables

Re: remove function pointer casts and constify function tables

[bfields]

Probably should have cc'd virtualization@lists.linux-foundation.org too.

On Fri, May 26, 2017 at 11:08:39AM -0400, bfields@fieldses.org wrote:
> On Tue, May 23, 2017 at 08:23:34AM -0400, bfields@fieldses.org wrote:
> > Unfortunately I can't get anything through testing.  It's not your
> > patches, it's something in -rc1.  My server VM stops responding to
> > any network traffic randomly in the middle of a run.  If I log in from a
> > serial console, I see the interface is up and everything looks OK.  I
> > haven't had the chance to do much more, and I'm not sure where to
> > start....  I started a git-bisect attempt, but there are several
> > unrelated problems, and I'm not sure this one is 100% reproduceable.
> 
> It looks like it may be due to something pulled in with virtio updates.
> I've reproduced the problem on c8b0d7290657 "s390/virtio: change
> maintainership" but not on v4.11.  Are there any known issues with those
> commits?
> 
> I've just been doing this long-running bisect while working on other
> stuff.  My reproducer (basically just running a bunch of NFS
> connectathon tests over a variety of protocol versions and security
> flavors) doesn't hit the bug reliably, and I've had to restart a couple
> times probably due to false negatives.  But this looks pretty promising,
> and there's only 17 commits in that range, so I'll keep bisecting.
> 
> --b.

Re: remove function pointer casts and constify function tables

[bfields]

Looks like the culprit is very likely d85b758f72b0 "virtio_net: fix
support for small rings".

After that patch, my NFS server VM stops responding to packets after a
few minutes of testing.  Before that patch, my server keeps working.

--b.

Re: remove function pointer casts and constify function tables

[Michael S. Tsirkin]

On Fri, May 26, 2017 at 03:31:33PM -0400, bfields@fieldses.org wrote:
> Looks like the culprit is very likely d85b758f72b0 "virtio_net: fix
> support for small rings".
> 
> After that patch, my NFS server VM stops responding to packets after a
> few minutes of testing.  Before that patch, my server keeps working.
> 
> --b.

Others complained about that too.
I'm still trying to reproduce though.

Meanwhile, could you please locate this line of code:
+               vi->rq[i].min_buf_len = mergeable_min_buf_len(vi, vi->rq[i].vq);

and add something like
        printk(KERN_ERR, "min buf = 0x%x expected 0x%x size 0x%x big %d\n",
               vi->rq[i].min_buf_len, GOOD_PACKET_LEN,
               virtqueue_get_vring_size(vi->rq[i].vq),
               (int)vi->big_packets);

after it?
Then boot and capture the output.

Thanks!


-- 
MST

Re: remove function pointer casts and constify function tables

[Michael S. Tsirkin]

On Tue, May 30, 2017 at 07:26:37PM +0300, Michael S. Tsirkin wrote:
> On Fri, May 26, 2017 at 03:31:33PM -0400, bfields@fieldses.org wrote:
> > Looks like the culprit is very likely d85b758f72b0 "virtio_net: fix
> > support for small rings".
> > 
> > After that patch, my NFS server VM stops responding to packets after a
> > few minutes of testing.  Before that patch, my server keeps working.
> > 
> > --b.
> 
> Others complained about that too.
> I'm still trying to reproduce though.
> 
> Meanwhile, could you please locate this line of code:
> +               vi->rq[i].min_buf_len = mergeable_min_buf_len(vi, vi->rq[i].vq);
> 
> and add something like
>         printk(KERN_ERR, "min buf = 0x%x expected 0x%x size 0x%x big %d\n",
>                vi->rq[i].min_buf_len, GOOD_PACKET_LEN,
>                virtqueue_get_vring_size(vi->rq[i].vq),
>                (int)vi->big_packets);
> 
> after it?
> Then boot and capture the output.
> 
> Thanks!
> 

Also, can you pls print the mergeable_rx_buffer_size attribute from
sysfs for this device?


> -- 
> MST

Re: remove function pointer casts and constify function tables

[Michael S. Tsirkin]

On Fri, May 26, 2017 at 03:31:33PM -0400, bfields@fieldses.org wrote:
> Looks like the culprit is very likely d85b758f72b0 "virtio_net: fix
> support for small rings".
> 
> After that patch, my NFS server VM stops responding to packets after a
> few minutes of testing.  Before that patch, my server keeps working.
> 
> --b.


So I think I know what caused this: looks like some hypervisors
aren't prepared to deal with a situation where packet size
becomes very small.

But which hypervisors exactly? I'd like to know in order to detect these
and decide whether I blacklist bad ones or whitelist known-good ones.

Thanks!

-- 
MST

Re: remove function pointer casts and constify function tables

[bfields]

On Tue, May 30, 2017 at 07:58:06PM +0300, Michael S. Tsirkin wrote:
> On Tue, May 30, 2017 at 07:26:37PM +0300, Michael S. Tsirkin wrote:
> > On Fri, May 26, 2017 at 03:31:33PM -0400, bfields@fieldses.org wrote:
> > > Looks like the culprit is very likely d85b758f72b0 "virtio_net: fix
> > > support for small rings".
> > > 
> > > After that patch, my NFS server VM stops responding to packets after a
> > > few minutes of testing.  Before that patch, my server keeps working.
> > > 
> > > --b.
> > 
> > Others complained about that too.
> > I'm still trying to reproduce though.
> > 
> > Meanwhile, could you please locate this line of code:
> > +               vi->rq[i].min_buf_len = mergeable_min_buf_len(vi, vi->rq[i].vq);
> > 
> > and add something like
> >         printk(KERN_ERR, "min buf = 0x%x expected 0x%x size 0x%x big %d\n",
> >                vi->rq[i].min_buf_len, GOOD_PACKET_LEN,
> >                virtqueue_get_vring_size(vi->rq[i].vq),
> >                (int)vi->big_packets);
> > 
> > after it?
> > Then boot and capture the output.
> > 
> > Thanks!
> > 
> 
> Also, can you pls print the mergeable_rx_buffer_size attribute from
> sysfs for this device?

On 4.12-rc1:

	# cat
	# /sys/devices/pci0000:00/0000:00:03.0/virtio0/net/eth0/queues/rx-0/virtio_net/mergeable_rx_buffer_size
	320

--b.

Re: remove function pointer casts and constify function tables

[bfields]

On Tue, May 30, 2017 at 08:03:12PM +0300, Michael S. Tsirkin wrote:
> On Fri, May 26, 2017 at 03:31:33PM -0400, bfields@fieldses.org wrote:
> > Looks like the culprit is very likely d85b758f72b0 "virtio_net: fix
> > support for small rings".
> > 
> > After that patch, my NFS server VM stops responding to packets after a
> > few minutes of testing.  Before that patch, my server keeps working.
> > 
> > --b.
> 
> 
> So I think I know what caused this: looks like some hypervisors
> aren't prepared to deal with a situation where packet size
> becomes very small.
> 
> But which hypervisors exactly? I'd like to know in order to detect these
> and decide whether I blacklist bad ones or whitelist known-good ones.

I'm running this under KVM on a Fedora 25 host
(4.10.15-200.fc25.x86_64).  Let me know if any more details about the
setup would be useful.

--b.

Re: remove function pointer casts and constify function tables

[bfields]

On Tue, May 30, 2017 at 07:26:37PM +0300, Michael S. Tsirkin wrote:
> On Fri, May 26, 2017 at 03:31:33PM -0400, bfields@fieldses.org wrote:
> > Looks like the culprit is very likely d85b758f72b0 "virtio_net: fix
> > support for small rings".
> > 
> > After that patch, my NFS server VM stops responding to packets after a
> > few minutes of testing.  Before that patch, my server keeps working.
> > 
> > --b.
> 
> Others complained about that too.
> I'm still trying to reproduce though.
> 
> Meanwhile, could you please locate this line of code:
> +               vi->rq[i].min_buf_len = mergeable_min_buf_len(vi, vi->rq[i].vq);
> 
> and add something like
>         printk(KERN_ERR, "min buf = 0x%x expected 0x%x size 0x%x big %d\n",
>                vi->rq[i].min_buf_len, GOOD_PACKET_LEN,
>                virtqueue_get_vring_size(vi->rq[i].vq),
>                (int)vi->big_packets);
> 
> after it?
> Then boot and capture the output.

Doesn't look like that code's run on boot; apply the below, boot, and:

 $ dmesg|grep expected

gives no output.

--b.

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 9320d96a1632..b10014f7b480 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2212,6 +2212,10 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
        for (i = 0; i < vi->max_queue_pairs; i++) {
                vi->rq[i].vq = vqs[rxq2vq(i)];
                vi->rq[i].min_buf_len = mergeable_min_buf_len(vi, vi->rq[i].vq);
+               printk(KERN_ERR, "min buf = 0x%x expected 0x%x size 0x%x big %d\n",
+                               vi->rq[i].min_buf_len, GOOD_PACKET_LEN,
+                               virtqueue_get_vring_size(vi->rq[i].vq),
+                               (int)vi->big_packets);
                vi->sq[i].vq = vqs[txq2vq(i)];
        }

Re: remove function pointer casts and constify function tables

[bfields]

On Wed, May 31, 2017 at 05:09:23PM -0400, bfields@fieldses.org wrote:
> On Tue, May 30, 2017 at 07:26:37PM +0300, Michael S. Tsirkin wrote:
> > On Fri, May 26, 2017 at 03:31:33PM -0400, bfields@fieldses.org wrote:
> > > Looks like the culprit is very likely d85b758f72b0 "virtio_net: fix
> > > support for small rings".
> > > 
> > > After that patch, my NFS server VM stops responding to packets after a
> > > few minutes of testing.  Before that patch, my server keeps working.
> > > 
> > > --b.
> > 
> > Others complained about that too.
> > I'm still trying to reproduce though.
> > 
> > Meanwhile, could you please locate this line of code:
> > +               vi->rq[i].min_buf_len = mergeable_min_buf_len(vi, vi->rq[i].vq);
> > 
> > and add something like
> >         printk(KERN_ERR, "min buf = 0x%x expected 0x%x size 0x%x big %d\n",
> >                vi->rq[i].min_buf_len, GOOD_PACKET_LEN,
> >                virtqueue_get_vring_size(vi->rq[i].vq),
> >                (int)vi->big_packets);
> > 
> > after it?
> > Then boot and capture the output.
> 
> Doesn't look like that code's run on boot; apply the below, boot, and:

Whoops, no, just a typo in the printk.  Here you go:

	min buf = 0x101 expected 0x5ee size 0x100 big 1

--b.