Re: [PATCH V5 0/9] Fixes for vhost metadata acceleration
On Tue, Aug 20, 2019 at 10:29:32AM +0800, Jason Wang wrote: > > On 2019/8/20 上午5:08, Michael S. Tsirkin wrote: > > On Tue, Aug 13, 2019 at 04:12:49PM +0800, Jason Wang wrote: > > > On 2019/8/12 下午5:49, Michael S. Tsirkin wrote: > > > > On Mon, Aug 12, 2019 at 10:44:51AM +0800, Jason Wang wrote: > > > > > On 2019/8/11 上午1:52, Michael S. Tsirkin wrote: > > > > > > On Fri, Aug 09, 2019 at 01:48:42AM -0400, Jason Wang wrote: > > > > > > > Hi all: > > > > > > > > > > > > > > This series try to fix several issues introduced by meta data > > > > > > > accelreation series. Please review. > > > > > > > > > > > > > > Changes from V4: > > > > > > > - switch to use spinlock synchronize MMU notifier with accessors > > > > > > > > > > > > > > Changes from V3: > > > > > > > - remove the unnecessary patch > > > > > > > > > > > > > > Changes from V2: > > > > > > > - use seqlck helper to synchronize MMU notifier with vhost worker > > > > > > > > > > > > > > Changes from V1: > > > > > > > - try not use RCU to syncrhonize MMU notifier with vhost worker > > > > > > > - set dirty pages after no readers > > > > > > > - return -EAGAIN only when we find the range is overlapped with > > > > > > > metadata > > > > > > > > > > > > > > Jason Wang (9): > > > > > > > vhost: don't set uaddr for invalid address > > > > > > > vhost: validate MMU notifier registration > > > > > > > vhost: fix vhost map leak > > > > > > > vhost: reset invalidate_count in vhost_set_vring_num_addr() > > > > > > > vhost: mark dirty pages during map uninit > > > > > > > vhost: don't do synchronize_rcu() in vhost_uninit_vq_maps() > > > > > > > vhost: do not use RCU to synchronize MMU notifier with worker > > > > > > > vhost: correctly set dirty pages in MMU notifiers callback > > > > > > > vhost: do not return -EAGAIN for non blocking invalidation > > > > > > > too early > > > > > > > > > > > > > > drivers/vhost/vhost.c | 202 > > > > > > > +- > > > > > > > drivers/vhost/vhost.h | 6 +- > > > > > > > 2 files changed, 122 insertions(+), 86 deletions(-) > > > > > > This generally looks more solid. > > > > > > > > > > > > But this amounts to a significant overhaul of the code. > > > > > > > > > > > > At this point how about we revert > > > > > > 7f466032dc9e5a61217f22ea34b2df932786bbfc > > > > > > for this release, and then re-apply a corrected version > > > > > > for the next one? > > > > > If possible, consider we've actually disabled the feature. How about > > > > > just > > > > > queued those patches for next release? > > > > > > > > > > Thanks > > > > Sorry if I was unclear. My idea is that > > > > 1. I revert the disabled code > > > > 2. You send a patch readding it with all the fixes squashed > > > > 3. Maybe optimizations on top right away? > > > > 4. We queue *that* for next and see what happens. > > > > > > > > And the advantage over the patchy approach is that the current patches > > > > are hard to review. E.g. it's not reasonable to ask RCU guys to review > > > > the whole of vhost for RCU usage but it's much more reasonable to ask > > > > about a specific patch. > > > > > > Ok. Then I agree to revert. > > > > > > Thanks > > Great, so please send the following: > > - revert > > - squashed and fixed patch > > > Just to confirm, do you want me to send a single series or two? > > Thanks > One is fine. -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V5 0/9] Fixes for vhost metadata acceleration
On 2019/8/20 上午5:08, Michael S. Tsirkin wrote: On Tue, Aug 13, 2019 at 04:12:49PM +0800, Jason Wang wrote: On 2019/8/12 下午5:49, Michael S. Tsirkin wrote: On Mon, Aug 12, 2019 at 10:44:51AM +0800, Jason Wang wrote: On 2019/8/11 上午1:52, Michael S. Tsirkin wrote: On Fri, Aug 09, 2019 at 01:48:42AM -0400, Jason Wang wrote: Hi all: This series try to fix several issues introduced by meta data accelreation series. Please review. Changes from V4: - switch to use spinlock synchronize MMU notifier with accessors Changes from V3: - remove the unnecessary patch Changes from V2: - use seqlck helper to synchronize MMU notifier with vhost worker Changes from V1: - try not use RCU to syncrhonize MMU notifier with vhost worker - set dirty pages after no readers - return -EAGAIN only when we find the range is overlapped with metadata Jason Wang (9): vhost: don't set uaddr for invalid address vhost: validate MMU notifier registration vhost: fix vhost map leak vhost: reset invalidate_count in vhost_set_vring_num_addr() vhost: mark dirty pages during map uninit vhost: don't do synchronize_rcu() in vhost_uninit_vq_maps() vhost: do not use RCU to synchronize MMU notifier with worker vhost: correctly set dirty pages in MMU notifiers callback vhost: do not return -EAGAIN for non blocking invalidation too early drivers/vhost/vhost.c | 202 +- drivers/vhost/vhost.h | 6 +- 2 files changed, 122 insertions(+), 86 deletions(-) This generally looks more solid. But this amounts to a significant overhaul of the code. At this point how about we revert 7f466032dc9e5a61217f22ea34b2df932786bbfc for this release, and then re-apply a corrected version for the next one? If possible, consider we've actually disabled the feature. How about just queued those patches for next release? Thanks Sorry if I was unclear. My idea is that 1. I revert the disabled code 2. You send a patch readding it with all the fixes squashed 3. Maybe optimizations on top right away? 4. We queue *that* for next and see what happens. And the advantage over the patchy approach is that the current patches are hard to review. E.g. it's not reasonable to ask RCU guys to review the whole of vhost for RCU usage but it's much more reasonable to ask about a specific patch. Ok. Then I agree to revert. Thanks Great, so please send the following: - revert - squashed and fixed patch Just to confirm, do you want me to send a single series or two? Thanks ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V5 0/9] Fixes for vhost metadata acceleration
On Tue, Aug 13, 2019 at 04:12:49PM +0800, Jason Wang wrote: > > On 2019/8/12 下午5:49, Michael S. Tsirkin wrote: > > On Mon, Aug 12, 2019 at 10:44:51AM +0800, Jason Wang wrote: > > > On 2019/8/11 上午1:52, Michael S. Tsirkin wrote: > > > > On Fri, Aug 09, 2019 at 01:48:42AM -0400, Jason Wang wrote: > > > > > Hi all: > > > > > > > > > > This series try to fix several issues introduced by meta data > > > > > accelreation series. Please review. > > > > > > > > > > Changes from V4: > > > > > - switch to use spinlock synchronize MMU notifier with accessors > > > > > > > > > > Changes from V3: > > > > > - remove the unnecessary patch > > > > > > > > > > Changes from V2: > > > > > - use seqlck helper to synchronize MMU notifier with vhost worker > > > > > > > > > > Changes from V1: > > > > > - try not use RCU to syncrhonize MMU notifier with vhost worker > > > > > - set dirty pages after no readers > > > > > - return -EAGAIN only when we find the range is overlapped with > > > > > metadata > > > > > > > > > > Jason Wang (9): > > > > > vhost: don't set uaddr for invalid address > > > > > vhost: validate MMU notifier registration > > > > > vhost: fix vhost map leak > > > > > vhost: reset invalidate_count in vhost_set_vring_num_addr() > > > > > vhost: mark dirty pages during map uninit > > > > > vhost: don't do synchronize_rcu() in vhost_uninit_vq_maps() > > > > > vhost: do not use RCU to synchronize MMU notifier with worker > > > > > vhost: correctly set dirty pages in MMU notifiers callback > > > > > vhost: do not return -EAGAIN for non blocking invalidation too > > > > > early > > > > > > > > > >drivers/vhost/vhost.c | 202 > > > > > +- > > > > >drivers/vhost/vhost.h | 6 +- > > > > >2 files changed, 122 insertions(+), 86 deletions(-) > > > > This generally looks more solid. > > > > > > > > But this amounts to a significant overhaul of the code. > > > > > > > > At this point how about we revert > > > > 7f466032dc9e5a61217f22ea34b2df932786bbfc > > > > for this release, and then re-apply a corrected version > > > > for the next one? > > > > > > If possible, consider we've actually disabled the feature. How about just > > > queued those patches for next release? > > > > > > Thanks > > Sorry if I was unclear. My idea is that > > 1. I revert the disabled code > > 2. You send a patch readding it with all the fixes squashed > > 3. Maybe optimizations on top right away? > > 4. We queue *that* for next and see what happens. > > > > And the advantage over the patchy approach is that the current patches > > are hard to review. E.g. it's not reasonable to ask RCU guys to review > > the whole of vhost for RCU usage but it's much more reasonable to ask > > about a specific patch. > > > Ok. Then I agree to revert. > > Thanks Great, so please send the following: - revert - squashed and fixed patch -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V5 0/9] Fixes for vhost metadata acceleration
On Thu, Aug 15, 2019 at 11:26:46AM +0800, Jason Wang wrote:
>
> On 2019/8/13 下午7:57, Jason Gunthorpe wrote:
> > On Tue, Aug 13, 2019 at 04:31:07PM +0800, Jason Wang wrote:
> >
> > > What kind of issues do you see? Spinlock is to synchronize GUP with MMU
> > > notifier in this series.
> > A GUP that can't sleep can't pagefault which makes it a really weird
> > pattern
>
>
> My understanding is __get_user_pages_fast() assumes caller can fail or have
> fallback. And we have graceful fallback to copy_{to|from}_user().
My point is that if you can fall back to copy_user then it is weird to
call the special non-sleeping GUP under a spinlock.
AFAIK the only reason this is done is because of the way the notifier
is being locked...
Jason
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V5 0/9] Fixes for vhost metadata acceleration
On 2019/8/14 上午12:41, Christoph Hellwig wrote: On Tue, Aug 13, 2019 at 08:57:07AM -0300, Jason Gunthorpe wrote: On Tue, Aug 13, 2019 at 04:31:07PM +0800, Jason Wang wrote: What kind of issues do you see? Spinlock is to synchronize GUP with MMU notifier in this series. A GUP that can't sleep can't pagefault which makes it a really weird pattern get_user_pages/get_user_pages_fast must not be called under a spinlock. We have the somewhat misnamed __get_user_page_fast that just does a lookup for existing pages and never faults for a few places that need to do that lookup from contexts where we can't sleep. Yes, I do use __get_user_pages_fast() in the code. Thanks ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V5 0/9] Fixes for vhost metadata acceleration
On 2019/8/13 下午7:57, Jason Gunthorpe wrote:
On Tue, Aug 13, 2019 at 04:31:07PM +0800, Jason Wang wrote:
What kind of issues do you see? Spinlock is to synchronize GUP with MMU
notifier in this series.
A GUP that can't sleep can't pagefault which makes it a really weird
pattern
My understanding is __get_user_pages_fast() assumes caller can fail or
have fallback. And we have graceful fallback to copy_{to|from}_user().
Btw, back to the original question. May I know why synchronize_rcu() is not
suitable? Consider:
We already went over this. You'd need to determine it doesn't somehow
deadlock the mm on reclaim paths. Maybe it is OK, the rcq_gq_wq is
marked WQ_MEM_RECLAIM at least..
Yes, will take a look at this.
I also think Michael was concerned about the latency spikes a long RCU
delay would cause.
I don't think it's a real problem consider MMU notifier could be
preempted or blocked.
Thanks
Jason
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V5 0/9] Fixes for vhost metadata acceleration
On Tue, Aug 13, 2019 at 08:57:07AM -0300, Jason Gunthorpe wrote: > On Tue, Aug 13, 2019 at 04:31:07PM +0800, Jason Wang wrote: > > > What kind of issues do you see? Spinlock is to synchronize GUP with MMU > > notifier in this series. > > A GUP that can't sleep can't pagefault which makes it a really weird > pattern get_user_pages/get_user_pages_fast must not be called under a spinlock. We have the somewhat misnamed __get_user_page_fast that just does a lookup for existing pages and never faults for a few places that need to do that lookup from contexts where we can't sleep. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V5 0/9] Fixes for vhost metadata acceleration
On Tue, Aug 13, 2019 at 04:31:07PM +0800, Jason Wang wrote: > What kind of issues do you see? Spinlock is to synchronize GUP with MMU > notifier in this series. A GUP that can't sleep can't pagefault which makes it a really weird pattern > Btw, back to the original question. May I know why synchronize_rcu() is not > suitable? Consider: We already went over this. You'd need to determine it doesn't somehow deadlock the mm on reclaim paths. Maybe it is OK, the rcq_gq_wq is marked WQ_MEM_RECLAIM at least.. I also think Michael was concerned about the latency spikes a long RCU delay would cause. Jason ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V5 0/9] Fixes for vhost metadata acceleration
On 2019/8/12 下午9:02, Jason Gunthorpe wrote: On Mon, Aug 12, 2019 at 05:49:08AM -0400, Michael S. Tsirkin wrote: On Mon, Aug 12, 2019 at 10:44:51AM +0800, Jason Wang wrote: On 2019/8/11 上午1:52, Michael S. Tsirkin wrote: On Fri, Aug 09, 2019 at 01:48:42AM -0400, Jason Wang wrote: Hi all: This series try to fix several issues introduced by meta data accelreation series. Please review. Changes from V4: - switch to use spinlock synchronize MMU notifier with accessors Changes from V3: - remove the unnecessary patch Changes from V2: - use seqlck helper to synchronize MMU notifier with vhost worker Changes from V1: - try not use RCU to syncrhonize MMU notifier with vhost worker - set dirty pages after no readers - return -EAGAIN only when we find the range is overlapped with metadata Jason Wang (9): vhost: don't set uaddr for invalid address vhost: validate MMU notifier registration vhost: fix vhost map leak vhost: reset invalidate_count in vhost_set_vring_num_addr() vhost: mark dirty pages during map uninit vhost: don't do synchronize_rcu() in vhost_uninit_vq_maps() vhost: do not use RCU to synchronize MMU notifier with worker vhost: correctly set dirty pages in MMU notifiers callback vhost: do not return -EAGAIN for non blocking invalidation too early drivers/vhost/vhost.c | 202 +- drivers/vhost/vhost.h | 6 +- 2 files changed, 122 insertions(+), 86 deletions(-) This generally looks more solid. But this amounts to a significant overhaul of the code. At this point how about we revert 7f466032dc9e5a61217f22ea34b2df932786bbfc for this release, and then re-apply a corrected version for the next one? If possible, consider we've actually disabled the feature. How about just queued those patches for next release? Thanks Sorry if I was unclear. My idea is that 1. I revert the disabled code 2. You send a patch readding it with all the fixes squashed 3. Maybe optimizations on top right away? 4. We queue *that* for next and see what happens. And the advantage over the patchy approach is that the current patches are hard to review. E.g. it's not reasonable to ask RCU guys to review the whole of vhost for RCU usage but it's much more reasonable to ask about a specific patch. I think there are other problems here too, I don't like that the use of mmu notifiers is so different from every other driver, or that GUP is called under spinlock. What kind of issues do you see? Spinlock is to synchronize GUP with MMU notifier in this series. Btw, back to the original question. May I know why synchronize_rcu() is not suitable? Consider: - MMU notifier are allowed to sleep - MMU notifier could be preempted If you mean something that prevents RCU grace period from running. I'm afraid MMU notifier is not the only victim. But it should be no more worse than some one is holding a lock for very long time. If the only concern is the preemption of vhost kthread, I can switch to use rcu_read_lock_bh() instead. Thanks So I favor the revert and try again approach as well. It is hard to get a clear picture with these endless bug fix patches Jason Ok. Thanks ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V5 0/9] Fixes for vhost metadata acceleration
On 2019/8/12 下午5:49, Michael S. Tsirkin wrote: On Mon, Aug 12, 2019 at 10:44:51AM +0800, Jason Wang wrote: On 2019/8/11 上午1:52, Michael S. Tsirkin wrote: On Fri, Aug 09, 2019 at 01:48:42AM -0400, Jason Wang wrote: Hi all: This series try to fix several issues introduced by meta data accelreation series. Please review. Changes from V4: - switch to use spinlock synchronize MMU notifier with accessors Changes from V3: - remove the unnecessary patch Changes from V2: - use seqlck helper to synchronize MMU notifier with vhost worker Changes from V1: - try not use RCU to syncrhonize MMU notifier with vhost worker - set dirty pages after no readers - return -EAGAIN only when we find the range is overlapped with metadata Jason Wang (9): vhost: don't set uaddr for invalid address vhost: validate MMU notifier registration vhost: fix vhost map leak vhost: reset invalidate_count in vhost_set_vring_num_addr() vhost: mark dirty pages during map uninit vhost: don't do synchronize_rcu() in vhost_uninit_vq_maps() vhost: do not use RCU to synchronize MMU notifier with worker vhost: correctly set dirty pages in MMU notifiers callback vhost: do not return -EAGAIN for non blocking invalidation too early drivers/vhost/vhost.c | 202 +- drivers/vhost/vhost.h | 6 +- 2 files changed, 122 insertions(+), 86 deletions(-) This generally looks more solid. But this amounts to a significant overhaul of the code. At this point how about we revert 7f466032dc9e5a61217f22ea34b2df932786bbfc for this release, and then re-apply a corrected version for the next one? If possible, consider we've actually disabled the feature. How about just queued those patches for next release? Thanks Sorry if I was unclear. My idea is that 1. I revert the disabled code 2. You send a patch readding it with all the fixes squashed 3. Maybe optimizations on top right away? 4. We queue *that* for next and see what happens. And the advantage over the patchy approach is that the current patches are hard to review. E.g. it's not reasonable to ask RCU guys to review the whole of vhost for RCU usage but it's much more reasonable to ask about a specific patch. Ok. Then I agree to revert. Thanks ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V5 0/9] Fixes for vhost metadata acceleration
On Mon, Aug 12, 2019 at 05:49:08AM -0400, Michael S. Tsirkin wrote: > On Mon, Aug 12, 2019 at 10:44:51AM +0800, Jason Wang wrote: > > > > On 2019/8/11 上午1:52, Michael S. Tsirkin wrote: > > > On Fri, Aug 09, 2019 at 01:48:42AM -0400, Jason Wang wrote: > > > > Hi all: > > > > > > > > This series try to fix several issues introduced by meta data > > > > accelreation series. Please review. > > > > > > > > Changes from V4: > > > > - switch to use spinlock synchronize MMU notifier with accessors > > > > > > > > Changes from V3: > > > > - remove the unnecessary patch > > > > > > > > Changes from V2: > > > > - use seqlck helper to synchronize MMU notifier with vhost worker > > > > > > > > Changes from V1: > > > > - try not use RCU to syncrhonize MMU notifier with vhost worker > > > > - set dirty pages after no readers > > > > - return -EAGAIN only when we find the range is overlapped with > > > >metadata > > > > > > > > Jason Wang (9): > > > >vhost: don't set uaddr for invalid address > > > >vhost: validate MMU notifier registration > > > >vhost: fix vhost map leak > > > >vhost: reset invalidate_count in vhost_set_vring_num_addr() > > > >vhost: mark dirty pages during map uninit > > > >vhost: don't do synchronize_rcu() in vhost_uninit_vq_maps() > > > >vhost: do not use RCU to synchronize MMU notifier with worker > > > >vhost: correctly set dirty pages in MMU notifiers callback > > > >vhost: do not return -EAGAIN for non blocking invalidation too early > > > > > > > > drivers/vhost/vhost.c | 202 +- > > > > drivers/vhost/vhost.h | 6 +- > > > > 2 files changed, 122 insertions(+), 86 deletions(-) > > > This generally looks more solid. > > > > > > But this amounts to a significant overhaul of the code. > > > > > > At this point how about we revert 7f466032dc9e5a61217f22ea34b2df932786bbfc > > > for this release, and then re-apply a corrected version > > > for the next one? > > > > > > If possible, consider we've actually disabled the feature. How about just > > queued those patches for next release? > > > > Thanks > > Sorry if I was unclear. My idea is that > 1. I revert the disabled code > 2. You send a patch readding it with all the fixes squashed > 3. Maybe optimizations on top right away? > 4. We queue *that* for next and see what happens. > > And the advantage over the patchy approach is that the current patches > are hard to review. E.g. it's not reasonable to ask RCU guys to review > the whole of vhost for RCU usage but it's much more reasonable to ask > about a specific patch. I think there are other problems here too, I don't like that the use of mmu notifiers is so different from every other driver, or that GUP is called under spinlock. So I favor the revert and try again approach as well. It is hard to get a clear picture with these endless bug fix patches Jason ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V5 0/9] Fixes for vhost metadata acceleration
On Mon, Aug 12, 2019 at 10:44:51AM +0800, Jason Wang wrote: > > On 2019/8/11 上午1:52, Michael S. Tsirkin wrote: > > On Fri, Aug 09, 2019 at 01:48:42AM -0400, Jason Wang wrote: > > > Hi all: > > > > > > This series try to fix several issues introduced by meta data > > > accelreation series. Please review. > > > > > > Changes from V4: > > > - switch to use spinlock synchronize MMU notifier with accessors > > > > > > Changes from V3: > > > - remove the unnecessary patch > > > > > > Changes from V2: > > > - use seqlck helper to synchronize MMU notifier with vhost worker > > > > > > Changes from V1: > > > - try not use RCU to syncrhonize MMU notifier with vhost worker > > > - set dirty pages after no readers > > > - return -EAGAIN only when we find the range is overlapped with > > >metadata > > > > > > Jason Wang (9): > > >vhost: don't set uaddr for invalid address > > >vhost: validate MMU notifier registration > > >vhost: fix vhost map leak > > >vhost: reset invalidate_count in vhost_set_vring_num_addr() > > >vhost: mark dirty pages during map uninit > > >vhost: don't do synchronize_rcu() in vhost_uninit_vq_maps() > > >vhost: do not use RCU to synchronize MMU notifier with worker > > >vhost: correctly set dirty pages in MMU notifiers callback > > >vhost: do not return -EAGAIN for non blocking invalidation too early > > > > > > drivers/vhost/vhost.c | 202 +- > > > drivers/vhost/vhost.h | 6 +- > > > 2 files changed, 122 insertions(+), 86 deletions(-) > > This generally looks more solid. > > > > But this amounts to a significant overhaul of the code. > > > > At this point how about we revert 7f466032dc9e5a61217f22ea34b2df932786bbfc > > for this release, and then re-apply a corrected version > > for the next one? > > > If possible, consider we've actually disabled the feature. How about just > queued those patches for next release? > > Thanks Sorry if I was unclear. My idea is that 1. I revert the disabled code 2. You send a patch readding it with all the fixes squashed 3. Maybe optimizations on top right away? 4. We queue *that* for next and see what happens. And the advantage over the patchy approach is that the current patches are hard to review. E.g. it's not reasonable to ask RCU guys to review the whole of vhost for RCU usage but it's much more reasonable to ask about a specific patch. -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V5 0/9] Fixes for vhost metadata acceleration
From: Jason Wang Date: Mon, 12 Aug 2019 10:44:51 +0800 > On 2019/8/11 上午1:52, Michael S. Tsirkin wrote: >> At this point how about we revert >> 7f466032dc9e5a61217f22ea34b2df932786bbfc >> for this release, and then re-apply a corrected version >> for the next one? > > If possible, consider we've actually disabled the feature. How about > just queued those patches for next release? I'm tossing this series while you and Michael decide how to move forward. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V5 0/9] Fixes for vhost metadata acceleration
On 2019/8/11 上午1:52, Michael S. Tsirkin wrote: On Fri, Aug 09, 2019 at 01:48:42AM -0400, Jason Wang wrote: Hi all: This series try to fix several issues introduced by meta data accelreation series. Please review. Changes from V4: - switch to use spinlock synchronize MMU notifier with accessors Changes from V3: - remove the unnecessary patch Changes from V2: - use seqlck helper to synchronize MMU notifier with vhost worker Changes from V1: - try not use RCU to syncrhonize MMU notifier with vhost worker - set dirty pages after no readers - return -EAGAIN only when we find the range is overlapped with metadata Jason Wang (9): vhost: don't set uaddr for invalid address vhost: validate MMU notifier registration vhost: fix vhost map leak vhost: reset invalidate_count in vhost_set_vring_num_addr() vhost: mark dirty pages during map uninit vhost: don't do synchronize_rcu() in vhost_uninit_vq_maps() vhost: do not use RCU to synchronize MMU notifier with worker vhost: correctly set dirty pages in MMU notifiers callback vhost: do not return -EAGAIN for non blocking invalidation too early drivers/vhost/vhost.c | 202 +- drivers/vhost/vhost.h | 6 +- 2 files changed, 122 insertions(+), 86 deletions(-) This generally looks more solid. But this amounts to a significant overhaul of the code. At this point how about we revert 7f466032dc9e5a61217f22ea34b2df932786bbfc for this release, and then re-apply a corrected version for the next one? If possible, consider we've actually disabled the feature. How about just queued those patches for next release? Thanks -- 2.18.1 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V5 0/9] Fixes for vhost metadata acceleration
On Thu, Aug 08, 2019 at 08:54:54PM +0800, Jason Wang wrote: > I don't have any objection to convert to spinlock() but just want to > know if any case that the above smp_mb() + counter looks good to you? So how about we try this: - revert the original patch for this release - new safe patch with a spinlock for the next release - whatever improvements we can come up with on top Thoughts? Because I think this needs much more scrutiny than we can give an incremental patch. -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V5 0/9] Fixes for vhost metadata acceleration
On Fri, Aug 09, 2019 at 01:48:42AM -0400, Jason Wang wrote: > Hi all: > > This series try to fix several issues introduced by meta data > accelreation series. Please review. > > Changes from V4: > - switch to use spinlock synchronize MMU notifier with accessors > > Changes from V3: > - remove the unnecessary patch > > Changes from V2: > - use seqlck helper to synchronize MMU notifier with vhost worker > > Changes from V1: > - try not use RCU to syncrhonize MMU notifier with vhost worker > - set dirty pages after no readers > - return -EAGAIN only when we find the range is overlapped with > metadata > > Jason Wang (9): > vhost: don't set uaddr for invalid address > vhost: validate MMU notifier registration > vhost: fix vhost map leak > vhost: reset invalidate_count in vhost_set_vring_num_addr() > vhost: mark dirty pages during map uninit > vhost: don't do synchronize_rcu() in vhost_uninit_vq_maps() > vhost: do not use RCU to synchronize MMU notifier with worker > vhost: correctly set dirty pages in MMU notifiers callback > vhost: do not return -EAGAIN for non blocking invalidation too early > > drivers/vhost/vhost.c | 202 +- > drivers/vhost/vhost.h | 6 +- > 2 files changed, 122 insertions(+), 86 deletions(-) This generally looks more solid. But this amounts to a significant overhaul of the code. At this point how about we revert 7f466032dc9e5a61217f22ea34b2df932786bbfc for this release, and then re-apply a corrected version for the next one? > -- > 2.18.1 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
