From: Rusty Russell <rusty@rustcorp.com.au> To: "Michael S. Tsirkin" <m.s.tsirkin@gmail.com> Cc: netdev@vger.kernel.org, virtualization@lists.linux-foundation.org, kvm@vger.kernel.org, linux-kernel@vger.kernel.org, mingo@elte.hu, linux-mm@kvack.org, akpm@linux-foundation.org, hpa@zytor.com, gregory.haskins@gmail.com, s.hetze@linux-ag.com, Daniel Walker <dwalker@fifo99.com>, Eric Dumazet <eric.dumazet@gmail.com> Subject: Re: [PATCHv8 3/3] vhost_net: a kernel-level virtio server Date: Tue, 10 Nov 2009 11:38:20 +1030 [thread overview] Message-ID: <200911101138.20569.rusty@rustcorp.com.au> (raw) In-Reply-To: <8f53421d0911082310n1f5f487ew8c2c03d2e1d7ca5c@mail.gmail.com> On Mon, 9 Nov 2009 05:40:32 pm Michael S. Tsirkin wrote: > On Mon, Nov 9, 2009 at 8:17 AM, Rusty Russell <rusty@rustcorp.com.au> wrote: > > There's something about the 'acked' which rubs me the wrong way. > > "enabled_features" is perhaps a better term than "acked_features"; "acked" > > seems more a user point-of-view, "enabled" seems more driver POV? > > Hmm. Are you happy with the ioctl name? If yes I think being consistent > with that is important. I think in my original comments I noted that I preferred GET / SET, rather than GET/ACK. > > Actually, this looks wrong to me: > > > > + case VHOST_SET_VRING_BASE: > > ... > > + vq->avail_idx = vq->last_avail_idx = s.num; > > > > The last_avail_idx is part of the state of the driver. It needs to be saved > > and restored over susp/resume. > > > Exactly. That's what VHOST_GET/SET_VRING_BASE does. avail_idx is just a > cached value for notify on empty, so what this does is clear the value. Ah, you actually refresh it every time anyway. Hmm, could you do my poor brain a favor and either just get_user it in vhost_trigger_irq(), or call it 'cached_avail_idx' or something? > > The only reason it's not in the ring itself > > is because I figured the other side doesn't need to see it (which is true, but > > missed debugging opportunities as well as man-in-the-middle issues like this > > one). I had a patch which put this field at the end of the ring, I might > > resurrect it to avoid this problem. This is backwards compatible with all > > implementations. See patch at end. > > Yes, I remember that patch. There seems to be little point though, at > this stage. Well, it avoids this ioctl, by exposing all the state. We may well need it later, to expand the ring in other ways. > > I would drop avail_idx altogether: get_user is basically free, and simplifies > > a lot. As most state is in the ring, all you need is an ioctl to save/restore > > the last_avail_idx. > > avail_idx is there for notify on empty: I had this thought that it's > better to leave the avail cache line alone when we are triggering > interrupt to avoid bouncing it around if guest is updating it meanwhile > on another CPU, and I think my testing showed that it helped > performance, but could be a mistake. You don't believe this can help? I believe it could help, but this is YA case where it would have been nice to have a dumb basic patch and this as a patch on top. But I am going to ask you to re-run that measurement, see if it stacks up (because it's an interesting lesson if it does..) Thanks! Rusty.
WARNING: multiple messages have this Message-ID (diff)
From: Rusty Russell <rusty@rustcorp.com.au> To: "Michael S. Tsirkin" <m.s.tsirkin@gmail.com> Cc: netdev@vger.kernel.org, virtualization@lists.linux-foundation.org, kvm@vger.kernel.org, linux-kernel@vger.kernel.org, mingo@elte.hu, linux-mm@kvack.org, akpm@linux-foundation.org, hpa@zytor.com, gregory.haskins@gmail.com, s.hetze@linux-ag.com, Daniel Walker <dwalker@fifo99.com>, Eric Dumazet <eric.dumazet@gmail.com> Subject: Re: [PATCHv8 3/3] vhost_net: a kernel-level virtio server Date: Tue, 10 Nov 2009 11:38:20 +1030 [thread overview] Message-ID: <200911101138.20569.rusty@rustcorp.com.au> (raw) In-Reply-To: <8f53421d0911082310n1f5f487ew8c2c03d2e1d7ca5c@mail.gmail.com> On Mon, 9 Nov 2009 05:40:32 pm Michael S. Tsirkin wrote: > On Mon, Nov 9, 2009 at 8:17 AM, Rusty Russell <rusty@rustcorp.com.au> wrote: > > There's something about the 'acked' which rubs me the wrong way. > > "enabled_features" is perhaps a better term than "acked_features"; "acked" > > seems more a user point-of-view, "enabled" seems more driver POV? > > Hmm. Are you happy with the ioctl name? If yes I think being consistent > with that is important. I think in my original comments I noted that I preferred GET / SET, rather than GET/ACK. > > Actually, this looks wrong to me: > > > > + case VHOST_SET_VRING_BASE: > > ... > > + vq->avail_idx = vq->last_avail_idx = s.num; > > > > The last_avail_idx is part of the state of the driver. It needs to be saved > > and restored over susp/resume. > > > Exactly. That's what VHOST_GET/SET_VRING_BASE does. avail_idx is just a > cached value for notify on empty, so what this does is clear the value. Ah, you actually refresh it every time anyway. Hmm, could you do my poor brain a favor and either just get_user it in vhost_trigger_irq(), or call it 'cached_avail_idx' or something? > > The only reason it's not in the ring itself > > is because I figured the other side doesn't need to see it (which is true, but > > missed debugging opportunities as well as man-in-the-middle issues like this > > one). I had a patch which put this field at the end of the ring, I might > > resurrect it to avoid this problem. This is backwards compatible with all > > implementations. See patch at end. > > Yes, I remember that patch. There seems to be little point though, at > this stage. Well, it avoids this ioctl, by exposing all the state. We may well need it later, to expand the ring in other ways. > > I would drop avail_idx altogether: get_user is basically free, and simplifies > > a lot. As most state is in the ring, all you need is an ioctl to save/restore > > the last_avail_idx. > > avail_idx is there for notify on empty: I had this thought that it's > better to leave the avail cache line alone when we are triggering > interrupt to avoid bouncing it around if guest is updating it meanwhile > on another CPU, and I think my testing showed that it helped > performance, but could be a mistake. You don't believe this can help? I believe it could help, but this is YA case where it would have been nice to have a dumb basic patch and this as a patch on top. But I am going to ask you to re-run that measurement, see if it stacks up (because it's an interesting lesson if it does..) Thanks! Rusty. -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
next prev parent reply other threads:[~2009-11-10 1:08 UTC|newest] Thread overview: 51+ messages / expand[flat|nested] mbox.gz Atom feed top [not found] <cover.1257349249.git.mst@redhat.com> 2009-11-04 15:55 ` [PATCHv8 1/3] tun: export underlying socket Michael S. Tsirkin 2009-11-04 15:55 ` Michael S. Tsirkin 2009-11-04 15:55 ` Michael S. Tsirkin 2009-11-04 15:55 ` Michael S. Tsirkin 2009-11-04 15:55 ` [PATCHv8 2/3] mm: export use_mm/unuse_mm to modules Michael S. Tsirkin 2009-11-04 15:55 ` Michael S. Tsirkin 2009-11-04 15:55 ` Michael S. Tsirkin 2009-11-04 15:55 ` Michael S. Tsirkin 2009-11-04 15:57 ` [PATCHv8 3/3] vhost_net: a kernel-level virtio server Michael S. Tsirkin 2009-11-04 15:57 ` Michael S. Tsirkin 2009-11-04 15:57 ` Michael S. Tsirkin 2009-11-04 15:57 ` Michael S. Tsirkin 2009-11-06 4:59 ` Rusty Russell 2009-11-06 4:59 ` Rusty Russell 2009-11-06 4:59 ` Rusty Russell 2009-11-08 11:35 ` Michael S. Tsirkin 2009-11-08 11:35 ` Michael S. Tsirkin 2009-11-09 6:17 ` Rusty Russell 2009-11-09 6:17 ` Rusty Russell 2009-11-09 6:17 ` Rusty Russell 2009-11-09 7:10 ` Michael S. Tsirkin 2009-11-09 7:10 ` Michael S. Tsirkin 2009-11-10 1:08 ` Rusty Russell [this message] 2009-11-10 1:08 ` Rusty Russell 2009-11-10 1:08 ` Rusty Russell 2009-11-09 7:10 ` Michael S. Tsirkin 2009-11-09 7:20 ` Michael S. Tsirkin 2009-11-09 7:20 ` Michael S. Tsirkin 2009-11-09 7:20 ` Michael S. Tsirkin 2009-11-09 11:55 ` Michael S. Tsirkin 2009-11-09 11:55 ` Michael S. Tsirkin 2009-11-09 11:55 ` Michael S. Tsirkin 2010-05-04 18:22 ` virtio: put last_used and last_avail index into ring itself Michael S. Tsirkin 2010-05-04 18:22 ` Michael S. Tsirkin 2010-05-06 0:52 ` Rusty Russell 2010-05-06 0:52 ` Rusty Russell 2010-05-06 0:52 ` Rusty Russell 2010-05-06 6:27 ` Michael S. Tsirkin 2010-05-06 6:27 ` Michael S. Tsirkin 2010-05-06 6:27 ` Michael S. Tsirkin 2010-05-07 3:05 ` Rusty Russell 2010-05-07 3:05 ` Rusty Russell 2010-05-07 3:05 ` Rusty Russell 2010-05-09 8:57 ` Michael S. Tsirkin 2010-05-09 8:57 ` Michael S. Tsirkin 2010-05-09 8:57 ` Michael S. Tsirkin 2010-05-10 3:11 ` Rusty Russell 2010-05-10 3:11 ` Rusty Russell 2010-05-10 3:11 ` Rusty Russell 2010-05-04 18:22 ` Michael S. Tsirkin 2009-11-08 11:35 ` [PATCHv8 3/3] vhost_net: a kernel-level virtio server Michael S. Tsirkin
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=200911101138.20569.rusty@rustcorp.com.au \ --to=rusty@rustcorp.com.au \ --cc=akpm@linux-foundation.org \ --cc=dwalker@fifo99.com \ --cc=eric.dumazet@gmail.com \ --cc=gregory.haskins@gmail.com \ --cc=hpa@zytor.com \ --cc=kvm@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=m.s.tsirkin@gmail.com \ --cc=mingo@elte.hu \ --cc=netdev@vger.kernel.org \ --cc=s.hetze@linux-ag.com \ --cc=virtualization@lists.linux-foundation.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.