* [PULL] virtio,vhost: cleanups and fixes
From: Michael S. Tsirkin @ 2019-03-10 3:44 UTC (permalink / raw)
To: Linus Torvalds
Cc: jroedel, kvm, arnd, mst, netdev, cohuck, linux-kernel,
virtualization, pasic
The following changes since commit 1c163f4c7b3f621efff9b28a47abb36f7378d783:
Linux 5.0 (2019-03-03 15:21:29 -0800)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git tags/for_linus
for you to fetch changes up to cfdbb4ed31aa777d59b288810f66eb06249ee5b7:
vhost: silence an unused-variable warning (2019-03-06 11:32:37 -0500)
----------------------------------------------------------------
virtio: fixes, cleanups
Several fixes, most notably fix for virtio on swiotlb systems.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
----------------------------------------------------------------
Arnd Bergmann (1):
vhost: silence an unused-variable warning
Cornelia Huck (3):
virtio-ccw: diag 500 may return a negative cookie
virtio-ccw: wire up ->bus_name callback
virtio: hint if callbacks surprisingly might sleep
Halil Pasic (1):
s390/virtio: handle find on invalid queue gracefully
Joerg Roedel (5):
swiotlb: Introduce swiotlb_max_mapping_size()
swiotlb: Add is_swiotlb_active() function
dma: Introduce dma_max_mapping_size()
virtio: Introduce virtio_max_dma_size()
virtio-blk: Consider virtio_max_dma_size() for maximum segment size
Wei Wang (2):
virtio-balloon: improve update_balloon_size_func
virtio_balloon: remove the unnecessary 0-initialization
Documentation/DMA-API.txt | 8 ++++++++
Documentation/virtual/kvm/s390-diag.txt | 3 ++-
drivers/block/virtio_blk.c | 10 ++++++----
drivers/s390/virtio/virtio_ccw.c | 12 +++++++++++-
drivers/vhost/vhost.c | 2 +-
drivers/virtio/virtio.c | 2 ++
drivers/virtio/virtio_balloon.c | 6 ++++--
drivers/virtio/virtio_ring.c | 11 +++++++++++
include/linux/dma-mapping.h | 8 ++++++++
include/linux/swiotlb.h | 11 +++++++++++
include/linux/virtio.h | 2 ++
include/linux/virtio_config.h | 13 +++++++++++++
kernel/dma/direct.c | 11 +++++++++++
kernel/dma/mapping.c | 14 ++++++++++++++
kernel/dma/swiotlb.c | 14 ++++++++++++++
15 files changed, 118 insertions(+), 9 deletions(-)
^ permalink raw reply
* Re: [PATCH] virtio_console: free unused buffers with virtio port
From: Pankaj Gupta @ 2019-03-11 5:58 UTC (permalink / raw)
To: linux-kernel, virtualization; +Cc: arnd, mst, Gal Hammer, gregkh, amit, xiaohli
In-Reply-To: <20190304130511.14450-1-pagupta@redhat.com>
>
> The commit a7a69ec0d8e4 ("virtio_console: free buffers after reset")
> deffered detaching of unused buffer to virtio device unplug time.
>
> This causes unplug/replug of single port in virtio device with an
> error "Error allocating inbufs\n". As we don't free the unused buffers
> attached with the port. Re-plug the same port tries to allocate new
> buffers in virtqueue and results in this error if queue is full.
>
> This patch removes the unused buffers in vq's when we unplug the port.
> This is the best we can do as we cannot call device_reset because virtio
> device is still active. This was the working behaviour before the change
> introduced in commit b3258ff1d6.
ping.
This patch fixes a regression in upstream kernel, virtio_console driver.
Can we please have this merged.
Thanks,
Pankaj
>
> Reported-by: Xiaohui Li <xiaohli@redhat.com>
> Fixes: b3258ff1d6 ("virtio_console: free buffers after reset")
> Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> ---
> drivers/char/virtio_console.c | 14 +++++++++++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index fbeb71953526..5fbf2ac73111 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -1506,15 +1506,25 @@ static void remove_port(struct kref *kref)
> kfree(port);
> }
>
> +static void remove_unused_bufs(struct virtqueue *vq)
> +{
> + struct port_buffer *buf;
> +
> + while ((buf = virtqueue_detach_unused_buf(vq)))
> + free_buf(buf, true);
> +}
> +
> static void remove_port_data(struct port *port)
> {
> spin_lock_irq(&port->inbuf_lock);
> /* Remove unused data this port might have received. */
> discard_port_data(port);
> + remove_unused_bufs(port->in_vq);
> spin_unlock_irq(&port->inbuf_lock);
>
> spin_lock_irq(&port->outvq_lock);
> reclaim_consumed_buffers(port);
> + remove_unused_bufs(port->out_vq);
> spin_unlock_irq(&port->outvq_lock);
> }
>
> @@ -1950,11 +1960,9 @@ static void remove_vqs(struct ports_device *portdev)
> struct virtqueue *vq;
>
> virtio_device_for_each_vq(portdev->vdev, vq) {
> - struct port_buffer *buf;
>
> flush_bufs(vq, true);
> - while ((buf = virtqueue_detach_unused_buf(vq)))
> - free_buf(buf, true);
> + remove_unused_bufs(vq);
> }
> portdev->vdev->config->del_vqs(portdev->vdev);
> kfree(portdev->in_vqs);
> --
> 2.20.1
>
>
^ permalink raw reply
* Re: [PATCH] drm/bochs: Fix NULL dereference on atomic_disable helper
From: Gerd Hoffmann @ 2019-03-11 6:22 UTC (permalink / raw)
To: Rodrigo Siqueira
Cc: David Airlie, linux-kernel, dri-devel, Daniel Vetter,
virtualization
In-Reply-To: <20190310212150.xhhb4abzm7j3ain2@smtp.gmail.com>
On Sun, Mar 10, 2019 at 06:21:50PM -0300, Rodrigo Siqueira wrote:
> When the subtest basic-plain-flip from IGT project was executed on Bochs
> driver, the following bug appears:
>
> BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
> ? drm_atomic_helper_commit_modeset_disables+0x396/0x3e0 [drm_kms_helper]
> +static void bochs_crtc_atomic_disable(struct drm_crtc *crtc,
> + struct drm_crtc_state *old_state)
> +{
> +}
> +
> static void bochs_crtc_atomic_flush(struct drm_crtc *crtc,
> struct drm_crtc_state *old_crtc_state)
> {
> @@ -66,6 +71,7 @@ static const struct drm_crtc_funcs bochs_crtc_funcs = {
> static const struct drm_crtc_helper_funcs bochs_helper_funcs = {
> .mode_set_nofb = bochs_crtc_mode_set_nofb,
> .atomic_enable = bochs_crtc_atomic_enable,
> + .atomic_disable = bochs_crtc_atomic_disable,
Shouldn't we make the callback optional instead of adding empty dummy
functions to drivers?
cheers,
Gerd
^ permalink raw reply
* Re: [PATCH] virtio_console: free unused buffers with virtio port
From: Michael S. Tsirkin @ 2019-03-11 6:35 UTC (permalink / raw)
To: Pankaj Gupta
Cc: arnd, amit, Gal Hammer, gregkh, linux-kernel, virtualization,
xiaohli
In-Reply-To: <444210313.9669582.1551769746169.JavaMail.zimbra@redhat.com>
On Tue, Mar 05, 2019 at 02:09:06AM -0500, Pankaj Gupta wrote:
>
> Hello Michael,
>
> Thanks for your reply.
>
> >
> > On Mon, Mar 04, 2019 at 06:35:11PM +0530, Pankaj Gupta wrote:
> > > The commit a7a69ec0d8e4 ("virtio_console: free buffers after reset")
> > > deffered detaching of unused buffer to virtio device unplug time.
> > >
> > > This causes unplug/replug of single port in virtio device with an
> > > error "Error allocating inbufs\n". As we don't free the unused buffers
> > > attached with the port. Re-plug the same port tries to allocate new
> > > buffers in virtqueue and results in this error if queue is full.
That's the basic issue, isn't it? Why aren't we
reusing buffers that are already there?
> > >
> > > This patch removes the unused buffers in vq's when we unplug the port.
> > > This is the best we can do as we cannot call device_reset because virtio
> > > device is still active. This was the working behaviour before the change
> > > introduced in commit b3258ff1d6.
> > >
> > > Reported-by: Xiaohui Li <xiaohli@redhat.com>
> > > Fixes: b3258ff1d6 ("virtio_console: free buffers after reset")
> > > Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> >
> > I think if you do this you need to add support
> > in the packed ring.
>
> o.k. I will look at the implementation details for "support
> of packed ring" for virtio_console. This will take some time.
>
> Meanwhile "virtio_console" port hotplug/unplug is broken in upstream.
> Can we accept this patch as it fixes the upstream and together
> with parent patch(b3258ff1d6) does nice cleanups as well.
>
> Thanks,
> Pankaj
Sorry, no - I don't think we should fix one configuration by breaking the other.
If you want to go back, then that's a spec violation, but I guess we can
fix the spec to match. OK, but code-wise if you call
virtqueue_detach_unused_buf without device reset then you need to teach
packed ring code to support that.
> >
> > > ---
> > > drivers/char/virtio_console.c | 14 +++++++++++---
> > > 1 file changed, 11 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> > > index fbeb71953526..5fbf2ac73111 100644
> > > --- a/drivers/char/virtio_console.c
> > > +++ b/drivers/char/virtio_console.c
> > > @@ -1506,15 +1506,25 @@ static void remove_port(struct kref *kref)
> > > kfree(port);
> > > }
> > >
> > > +static void remove_unused_bufs(struct virtqueue *vq)
> > > +{
> > > + struct port_buffer *buf;
> > > +
> > > + while ((buf = virtqueue_detach_unused_buf(vq)))
> > > + free_buf(buf, true);
> > > +}
> > > +
> > > static void remove_port_data(struct port *port)
> > > {
> > > spin_lock_irq(&port->inbuf_lock);
> > > /* Remove unused data this port might have received. */
> > > discard_port_data(port);
> > > + remove_unused_bufs(port->in_vq);
> > > spin_unlock_irq(&port->inbuf_lock);
> > >
> > > spin_lock_irq(&port->outvq_lock);
> > > reclaim_consumed_buffers(port);
> > > + remove_unused_bufs(port->out_vq);
> > > spin_unlock_irq(&port->outvq_lock);
> > > }
> > >
> > > @@ -1950,11 +1960,9 @@ static void remove_vqs(struct ports_device *portdev)
> > > struct virtqueue *vq;
> > >
> > > virtio_device_for_each_vq(portdev->vdev, vq) {
> > > - struct port_buffer *buf;
> > >
> > > flush_bufs(vq, true);
> > > - while ((buf = virtqueue_detach_unused_buf(vq)))
> > > - free_buf(buf, true);
> > > + remove_unused_bufs(vq);
> > > }
> > > portdev->vdev->config->del_vqs(portdev->vdev);
> > > kfree(portdev->in_vqs);
> > > --
> > > 2.20.1
> >
^ permalink raw reply
* Re: [PATCH] virtio_console: free unused buffers with virtio port
From: Michael S. Tsirkin @ 2019-03-11 6:36 UTC (permalink / raw)
To: Pankaj Gupta
Cc: arnd, amit, Gal Hammer, gregkh, linux-kernel, virtualization,
xiaohli
In-Reply-To: <1061079510.11168464.1552283891949.JavaMail.zimbra@redhat.com>
On Mon, Mar 11, 2019 at 01:58:11AM -0400, Pankaj Gupta wrote:
>
> >
> > The commit a7a69ec0d8e4 ("virtio_console: free buffers after reset")
> > deffered detaching of unused buffer to virtio device unplug time.
> >
> > This causes unplug/replug of single port in virtio device with an
> > error "Error allocating inbufs\n". As we don't free the unused buffers
> > attached with the port. Re-plug the same port tries to allocate new
> > buffers in virtqueue and results in this error if queue is full.
> >
> > This patch removes the unused buffers in vq's when we unplug the port.
> > This is the best we can do as we cannot call device_reset because virtio
> > device is still active. This was the working behaviour before the change
> > introduced in commit b3258ff1d6.
>
> ping.
>
> This patch fixes a regression in upstream kernel, virtio_console driver.
> Can we please have this merged.
>
> Thanks,
> Pankaj
Sorry, no, I don't think that's a full fix.
> >
> > Reported-by: Xiaohui Li <xiaohli@redhat.com>
> > Fixes: b3258ff1d6 ("virtio_console: free buffers after reset")
> > Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> > ---
> > drivers/char/virtio_console.c | 14 +++++++++++---
> > 1 file changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> > index fbeb71953526..5fbf2ac73111 100644
> > --- a/drivers/char/virtio_console.c
> > +++ b/drivers/char/virtio_console.c
> > @@ -1506,15 +1506,25 @@ static void remove_port(struct kref *kref)
> > kfree(port);
> > }
> >
> > +static void remove_unused_bufs(struct virtqueue *vq)
> > +{
> > + struct port_buffer *buf;
> > +
> > + while ((buf = virtqueue_detach_unused_buf(vq)))
> > + free_buf(buf, true);
> > +}
> > +
> > static void remove_port_data(struct port *port)
> > {
> > spin_lock_irq(&port->inbuf_lock);
> > /* Remove unused data this port might have received. */
> > discard_port_data(port);
> > + remove_unused_bufs(port->in_vq);
> > spin_unlock_irq(&port->inbuf_lock);
> >
> > spin_lock_irq(&port->outvq_lock);
> > reclaim_consumed_buffers(port);
> > + remove_unused_bufs(port->out_vq);
> > spin_unlock_irq(&port->outvq_lock);
> > }
> >
> > @@ -1950,11 +1960,9 @@ static void remove_vqs(struct ports_device *portdev)
> > struct virtqueue *vq;
> >
> > virtio_device_for_each_vq(portdev->vdev, vq) {
> > - struct port_buffer *buf;
> >
> > flush_bufs(vq, true);
> > - while ((buf = virtqueue_detach_unused_buf(vq)))
> > - free_buf(buf, true);
> > + remove_unused_bufs(vq);
> > }
> > portdev->vdev->config->del_vqs(portdev->vdev);
> > kfree(portdev->in_vqs);
> > --
> > 2.20.1
> >
> >
^ permalink raw reply
* Re: [PATCH] virtio_console: free unused buffers with virtio port
From: Pankaj Gupta @ 2019-03-11 6:53 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: arnd, amit, Gal Hammer, gregkh, linux-kernel, virtualization,
xiaohli
In-Reply-To: <20190311023154-mutt-send-email-mst@kernel.org>
> >
> > Hello Michael,
> >
> > Thanks for your reply.
> >
> > >
> > > On Mon, Mar 04, 2019 at 06:35:11PM +0530, Pankaj Gupta wrote:
> > > > The commit a7a69ec0d8e4 ("virtio_console: free buffers after reset")
> > > > deffered detaching of unused buffer to virtio device unplug time.
> > > >
> > > > This causes unplug/replug of single port in virtio device with an
> > > > error "Error allocating inbufs\n". As we don't free the unused
> > > > buffers
> > > > attached with the port. Re-plug the same port tries to allocate new
> > > > buffers in virtqueue and results in this error if queue is full.
>
> That's the basic issue, isn't it? Why aren't we
> reusing buffers that are already there?
I think that that's how initial design has been. Will see if I can fix this.
>
>
> > > >
> > > > This patch removes the unused buffers in vq's when we unplug the
> > > > port.
> > > > This is the best we can do as we cannot call device_reset because
> > > > virtio
> > > > device is still active. This was the working behaviour before the
> > > > change
> > > > introduced in commit b3258ff1d6.
> > > >
> > > > Reported-by: Xiaohui Li <xiaohli@redhat.com>
> > > > Fixes: b3258ff1d6 ("virtio_console: free buffers after reset")
> > > > Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> > >
> > > I think if you do this you need to add support
> > > in the packed ring.
> >
> > o.k. I will look at the implementation details for "support
> > of packed ring" for virtio_console. This will take some time.
> >
> > Meanwhile "virtio_console" port hotplug/unplug is broken in upstream.
> > Can we accept this patch as it fixes the upstream and together
> > with parent patch(b3258ff1d6) does nice cleanups as well.
> >
> > Thanks,
> > Pankaj
>
> Sorry, no - I don't think we should fix one configuration by breaking the
> other.
> If you want to go back, then that's a spec violation, but I guess we can
> fix the spec to match. OK, but code-wise if you call
> virtqueue_detach_unused_buf without device reset then you need to teach
> packed ring code to support that.
o.k. Will look at this.
Thanks for the pointers.
Thanks,
Pankaj
>
>
>
> > >
> > > > ---
> > > > drivers/char/virtio_console.c | 14 +++++++++++---
> > > > 1 file changed, 11 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/char/virtio_console.c
> > > > b/drivers/char/virtio_console.c
> > > > index fbeb71953526..5fbf2ac73111 100644
> > > > --- a/drivers/char/virtio_console.c
> > > > +++ b/drivers/char/virtio_console.c
> > > > @@ -1506,15 +1506,25 @@ static void remove_port(struct kref *kref)
> > > > kfree(port);
> > > > }
> > > >
> > > > +static void remove_unused_bufs(struct virtqueue *vq)
> > > > +{
> > > > + struct port_buffer *buf;
> > > > +
> > > > + while ((buf = virtqueue_detach_unused_buf(vq)))
> > > > + free_buf(buf, true);
> > > > +}
> > > > +
> > > > static void remove_port_data(struct port *port)
> > > > {
> > > > spin_lock_irq(&port->inbuf_lock);
> > > > /* Remove unused data this port might have received. */
> > > > discard_port_data(port);
> > > > + remove_unused_bufs(port->in_vq);
> > > > spin_unlock_irq(&port->inbuf_lock);
> > > >
> > > > spin_lock_irq(&port->outvq_lock);
> > > > reclaim_consumed_buffers(port);
> > > > + remove_unused_bufs(port->out_vq);
> > > > spin_unlock_irq(&port->outvq_lock);
> > > > }
> > > >
> > > > @@ -1950,11 +1960,9 @@ static void remove_vqs(struct ports_device
> > > > *portdev)
> > > > struct virtqueue *vq;
> > > >
> > > > virtio_device_for_each_vq(portdev->vdev, vq) {
> > > > - struct port_buffer *buf;
> > > >
> > > > flush_bufs(vq, true);
> > > > - while ((buf = virtqueue_detach_unused_buf(vq)))
> > > > - free_buf(buf, true);
> > > > + remove_unused_bufs(vq);
> > > > }
> > > > portdev->vdev->config->del_vqs(portdev->vdev);
> > > > kfree(portdev->in_vqs);
> > > > --
> > > > 2.20.1
> > >
>
^ permalink raw reply
* Re: [RFC PATCH V2 0/5] vhost: accelerate metadata access through vmap()
From: Jason Wang @ 2019-03-11 7:13 UTC (permalink / raw)
To: Christoph Hellwig
Cc: aarcange, linux-parisc, kvm, mst, netdev, linux-kernel,
virtualization, linux-mm, linux-arm-kernel
In-Reply-To: <20190308141220.GA21082@infradead.org>
On 2019/3/8 下午10:12, Christoph Hellwig wrote:
> On Wed, Mar 06, 2019 at 02:18:07AM -0500, Jason Wang wrote:
>> This series tries to access virtqueue metadata through kernel virtual
>> address instead of copy_user() friends since they had too much
>> overheads like checks, spec barriers or even hardware feature
>> toggling. This is done through setup kernel address through vmap() and
>> resigter MMU notifier for invalidation.
>>
>> Test shows about 24% improvement on TX PPS. TCP_STREAM doesn't see
>> obvious improvement.
> How is this going to work for CPUs with virtually tagged caches?
Anything different that you worry? I can have a test but do you know any
archs that use virtual tag cache?
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC PATCH V2 5/5] vhost: access vq metadata through kernel virtual address
From: Jason Wang @ 2019-03-11 7:18 UTC (permalink / raw)
To: Jerome Glisse
Cc: Andrea Arcangeli, kvm, Michael S. Tsirkin, netdev, linux-kernel,
virtualization, linux-mm
In-Reply-To: <20190308145800.GA3661@redhat.com>
On 2019/3/8 下午10:58, Jerome Glisse wrote:
> On Fri, Mar 08, 2019 at 04:50:36PM +0800, Jason Wang wrote:
>> On 2019/3/8 上午3:16, Andrea Arcangeli wrote:
>>> On Thu, Mar 07, 2019 at 12:56:45PM -0500, Michael S. Tsirkin wrote:
>>>> On Thu, Mar 07, 2019 at 10:47:22AM -0500, Michael S. Tsirkin wrote:
>>>>> On Wed, Mar 06, 2019 at 02:18:12AM -0500, Jason Wang wrote:
>>>>>> +static const struct mmu_notifier_ops vhost_mmu_notifier_ops = {
>>>>>> + .invalidate_range = vhost_invalidate_range,
>>>>>> +};
>>>>>> +
>>>>>> void vhost_dev_init(struct vhost_dev *dev,
>>>>>> struct vhost_virtqueue **vqs, int nvqs, int iov_limit)
>>>>>> {
>>>>> I also wonder here: when page is write protected then
>>>>> it does not look like .invalidate_range is invoked.
>>>>>
>>>>> E.g. mm/ksm.c calls
>>>>>
>>>>> mmu_notifier_invalidate_range_start and
>>>>> mmu_notifier_invalidate_range_end but not mmu_notifier_invalidate_range.
>>>>>
>>>>> Similarly, rmap in page_mkclean_one will not call
>>>>> mmu_notifier_invalidate_range.
>>>>>
>>>>> If I'm right vhost won't get notified when page is write-protected since you
>>>>> didn't install start/end notifiers. Note that end notifier can be called
>>>>> with page locked, so it's not as straight-forward as just adding a call.
>>>>> Writing into a write-protected page isn't a good idea.
>>>>>
>>>>> Note that documentation says:
>>>>> it is fine to delay the mmu_notifier_invalidate_range
>>>>> call to mmu_notifier_invalidate_range_end() outside the page table lock.
>>>>> implying it's called just later.
>>>> OK I missed the fact that _end actually calls
>>>> mmu_notifier_invalidate_range internally. So that part is fine but the
>>>> fact that you are trying to take page lock under VQ mutex and take same
>>>> mutex within notifier probably means it's broken for ksm and rmap at
>>>> least since these call invalidate with lock taken.
>>> Yes this lock inversion needs more thoughts.
>>>
>>>> And generally, Andrea told me offline one can not take mutex under
>>>> the notifier callback. I CC'd Andrea for why.
>>> Yes, the problem then is the ->invalidate_page is called then under PT
>>> lock so it cannot take mutex, you also cannot take the page_lock, it
>>> can at most take a spinlock or trylock_page.
>>>
>>> So it must switch back to the _start/_end methods unless you rewrite
>>> the locking.
>>>
>>> The difference with _start/_end, is that ->invalidate_range avoids the
>>> _start callback basically, but to avoid the _start callback safely, it
>>> has to be called in between the ptep_clear_flush and the set_pte_at
>>> whenever the pfn changes like during a COW. So it cannot be coalesced
>>> in a single TLB flush that invalidates all sptes in a range like we
>>> prefer for performance reasons for example in KVM. It also cannot
>>> sleep.
>>>
>>> In short ->invalidate_range must be really fast (it shouldn't require
>>> to send IPI to all other CPUs like KVM may require during an
>>> invalidate_range_start) and it must not sleep, in order to prefer it
>>> to _start/_end.
>>>
>>> I.e. the invalidate of the secondary MMU that walks the linux
>>> pagetables in hardware (in vhost case with GUP in software) has to
>>> happen while the linux pagetable is zero, otherwise a concurrent
>>> hardware pagetable lookup could re-instantiate a mapping to the old
>>> page in between the set_pte_at and the invalidate_range_end (which
>>> internally calls ->invalidate_range). Jerome documented it nicely in
>>> Documentation/vm/mmu_notifier.rst .
>>
>> Right, I've actually gone through this several times but some details were
>> missed by me obviously.
>>
>>
>>> Now you don't really walk the pagetable in hardware in vhost, but if
>>> you use gup_fast after usemm() it's similar.
>>>
>>> For vhost the invalidate would be really fast, there are no IPI to
>>> deliver at all, the problem is just the mutex.
>>
>> Yes. A possible solution is to introduce a valid flag for VA. Vhost may only
>> try to access kernel VA when it was valid. Invalidate_range_start() will
>> clear this under the protection of the vq mutex when it can block. Then
>> invalidate_range_end() then can clear this flag. An issue is blockable is
>> always false for range_end().
>>
> Note that there can be multiple asynchronous concurrent invalidate_range
> callbacks. So a flag does not work but a counter of number of active
> invalidation would. See how KSM is doing it for instance in kvm_main.c
>
> The pattern for this kind of thing is:
> my_invalidate_range_start(start,end) {
> ...
> if (mystruct_overlap(mystruct, start, end)) {
> mystruct_lock();
> mystruct->invalidate_count++;
> ...
> mystruct_unlock();
> }
> }
>
> my_invalidate_range_end(start,end) {
> ...
> if (mystruct_overlap(mystruct, start, end)) {
> mystruct_lock();
> mystruct->invalidate_count--;
> ...
> mystruct_unlock();
> }
> }
>
> my_access_va(mystruct) {
> again:
> wait_on(!mystruct->invalidate_count)
> mystruct_lock();
> if (mystruct->invalidate_count) {
> mystruct_unlock();
> goto again;
> }
> GUP();
> ...
> mystruct_unlock();
> }
>
> Cheers,
> Jérôme
Yes, this should work.
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC PATCH V2 5/5] vhost: access vq metadata through kernel virtual address
From: Jason Wang @ 2019-03-11 7:21 UTC (permalink / raw)
To: Andrea Arcangeli
Cc: Jan Kara, kvm, Michael S. Tsirkin, netdev, linux-kernel,
virtualization, linux-mm, Jerome Glisse
In-Reply-To: <20190308191108.GA26923@redhat.com>
On 2019/3/9 上午3:11, Andrea Arcangeli wrote:
> On Fri, Mar 08, 2019 at 05:13:26PM +0800, Jason Wang wrote:
>> Actually not wrapping around, the pages for used ring was marked as
>> dirty after a round of virtqueue processing when we're sure vhost wrote
>> something there.
> Thanks for the clarification. So we need to convert it to
> set_page_dirty and move it to the mmu notifier invalidate but in those
> cases where gup_fast was called with write=1 (1 out of 3).
>
> If using ->invalidate_range the page pin also must be removed
> immediately after get_user_pages returns (not ok to hold the pin in
> vmap until ->invalidate_range is called) to avoid false positive gup
> pin checks in things like KSM, or the pin must be released in
> invalidate_range_start (which is called before the pin checks).
>
> Here's why:
>
> /*
> * Check that no O_DIRECT or similar I/O is in progress on the
> * page
> */
> if (page_mapcount(page) + 1 + swapped != page_count(page)) {
> set_pte_at(mm, pvmw.address, pvmw.pte, entry);
> goto out_unlock;
> }
> [..]
> set_pte_at_notify(mm, pvmw.address, pvmw.pte, entry);
> ^^^^^^^ too late release the pin here, the
> above already failed
>
> ->invalidate_range cannot be used with mutex anyway so you need to go
> back with invalidate_range_start/end anyway, just the pin must be
> released in _start at the latest in such case.
Yes.
>
> My prefer is generally to call gup_fast() followed by immediate
> put_page() because I always want to drop FOLL_GET from gup_fast as a
> whole to avoid 2 useless atomic ops per gup_fast.
Ok, will do this (if I still plan to use vmap() in next version).
>
> I'll write more about vmap in answer to the other email.
>
> Thanks,
> Andrea
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC PATCH V2 5/5] vhost: access vq metadata through kernel virtual address
From: Jason Wang @ 2019-03-11 7:40 UTC (permalink / raw)
To: Andrea Arcangeli
Cc: kvm, Michael S. Tsirkin, netdev, linux-kernel, virtualization,
linux-mm, Jerome Glisse
In-Reply-To: <20190308194845.GC26923@redhat.com>
On 2019/3/9 上午3:48, Andrea Arcangeli wrote:
> Hello Jeson,
>
> On Fri, Mar 08, 2019 at 04:50:36PM +0800, Jason Wang wrote:
>> Just to make sure I understand here. For boosting through huge TLB, do
>> you mean we can do that in the future (e.g by mapping more userspace
>> pages to kenrel) or it can be done by this series (only about three 4K
>> pages were vmapped per virtqueue)?
> When I answered about the advantages of mmu notifier and I mentioned
> guaranteed 2m/gigapages where available, I overlooked the detail you
> were using vmap instead of kmap. So with vmap you're actually doing
> the opposite, it slows down the access because it will always use a 4k
> TLB even if QEMU runs on THP or gigapages hugetlbfs.
>
> If there's just one page (or a few pages) in each vmap there's no need
> of vmap, the linearity vmap provides doesn't pay off in such
> case.
>
> So likely there's further room for improvement here that you can
> achieve in the current series by just dropping vmap/vunmap.
>
> You can just use kmap (or kmap_atomic if you're in preemptible
> section, should work from bh/irq).
>
> In short the mmu notifier to invalidate only sets a "struct page *
> userringpage" pointer to NULL without calls to vunmap.
>
> In all cases immediately after gup_fast returns you can always call
> put_page immediately (which explains why I'd like an option to drop
> FOLL_GET from gup_fast to speed it up).
>
> Then you can check the sequence_counter and inc/dec counter increased
> by _start/_end. That will tell you if the page you got and you called
> put_page to immediately unpin it or even to free it, cannot go away
> under you until the invalidate is called.
>
> If sequence counters and counter tells that gup_fast raced with anyt
> mmu notifier invalidate you can just repeat gup_fast. Otherwise you're
> done, the page cannot go away under you, the host virtual to host
> physical mapping cannot change either. And the page is not pinned
> either. So you can just set the "struct page * userringpage = page"
> where "page" was the one setup by gup_fast.
>
> When later the invalidate runs, you can just call set_page_dirty if
> gup_fast was called with "write = 1" and then you clear the pointer
> "userringpage = NULL".
>
> When you need to read/write to the memory
> kmap/kmap_atomic(userringpage) should work.
Yes, I've considered kmap() from the start. The reason I don't do that
is large virtqueue may need more than one page so VA might not be
contiguous. But this is probably not a big issue which just need more
tricks in the vhost memory accessors.
>
> In short because there's no hardware involvement here, the established
> mapping is just the pointer to the page, there is no need of setting
> up any pagetables or to do any TLB flushes (except on 32bit archs if
> the page is above the direct mapping but it never happens on 64bit
> archs).
I see, I believe we don't care much about the performance of 32bit archs
(or we can just fallback to copy_to_user() friends). Using direct
mapping (I guess kernel will always try hugepage for that?) should be
better and we can even use it for the data transfer not only for the
metadata.
Thanks
>
> Thanks,
> Andrea
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC PATCH V2 5/5] vhost: access vq metadata through kernel virtual address
From: Michael S. Tsirkin @ 2019-03-11 12:48 UTC (permalink / raw)
To: Jason Wang
Cc: Andrea Arcangeli, kvm, netdev, linux-kernel, virtualization,
linux-mm, Jerome Glisse
In-Reply-To: <8b68a2a0-907a-15f5-a07f-fc5b53d7ea19@redhat.com>
On Mon, Mar 11, 2019 at 03:40:31PM +0800, Jason Wang wrote:
>
> On 2019/3/9 上午3:48, Andrea Arcangeli wrote:
> > Hello Jeson,
> >
> > On Fri, Mar 08, 2019 at 04:50:36PM +0800, Jason Wang wrote:
> > > Just to make sure I understand here. For boosting through huge TLB, do
> > > you mean we can do that in the future (e.g by mapping more userspace
> > > pages to kenrel) or it can be done by this series (only about three 4K
> > > pages were vmapped per virtqueue)?
> > When I answered about the advantages of mmu notifier and I mentioned
> > guaranteed 2m/gigapages where available, I overlooked the detail you
> > were using vmap instead of kmap. So with vmap you're actually doing
> > the opposite, it slows down the access because it will always use a 4k
> > TLB even if QEMU runs on THP or gigapages hugetlbfs.
> >
> > If there's just one page (or a few pages) in each vmap there's no need
> > of vmap, the linearity vmap provides doesn't pay off in such
> > case.
> >
> > So likely there's further room for improvement here that you can
> > achieve in the current series by just dropping vmap/vunmap.
> >
> > You can just use kmap (or kmap_atomic if you're in preemptible
> > section, should work from bh/irq).
> >
> > In short the mmu notifier to invalidate only sets a "struct page *
> > userringpage" pointer to NULL without calls to vunmap.
> >
> > In all cases immediately after gup_fast returns you can always call
> > put_page immediately (which explains why I'd like an option to drop
> > FOLL_GET from gup_fast to speed it up).
> >
> > Then you can check the sequence_counter and inc/dec counter increased
> > by _start/_end. That will tell you if the page you got and you called
> > put_page to immediately unpin it or even to free it, cannot go away
> > under you until the invalidate is called.
> >
> > If sequence counters and counter tells that gup_fast raced with anyt
> > mmu notifier invalidate you can just repeat gup_fast. Otherwise you're
> > done, the page cannot go away under you, the host virtual to host
> > physical mapping cannot change either. And the page is not pinned
> > either. So you can just set the "struct page * userringpage = page"
> > where "page" was the one setup by gup_fast.
> >
> > When later the invalidate runs, you can just call set_page_dirty if
> > gup_fast was called with "write = 1" and then you clear the pointer
> > "userringpage = NULL".
> >
> > When you need to read/write to the memory
> > kmap/kmap_atomic(userringpage) should work.
>
>
> Yes, I've considered kmap() from the start. The reason I don't do that is
> large virtqueue may need more than one page so VA might not be contiguous.
> But this is probably not a big issue which just need more tricks in the
> vhost memory accessors.
>
>
> >
> > In short because there's no hardware involvement here, the established
> > mapping is just the pointer to the page, there is no need of setting
> > up any pagetables or to do any TLB flushes (except on 32bit archs if
> > the page is above the direct mapping but it never happens on 64bit
> > archs).
>
>
> I see, I believe we don't care much about the performance of 32bit archs (or
> we can just fallback to copy_to_user() friends).
Using copyXuser is better I guess.
> Using direct mapping (I
> guess kernel will always try hugepage for that?) should be better and we can
> even use it for the data transfer not only for the metadata.
>
> Thanks
We can't really. The big issue is get user pages. Doing that on data
path will be slower than copyXuser. Or maybe it won't with the
amount of mitigations spread around. Go ahead and try.
>
> >
> > Thanks,
> > Andrea
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH] drm/bochs: Fix NULL dereference on atomic_disable helper
From: Gerd Hoffmann @ 2019-03-11 13:07 UTC (permalink / raw)
To: Rodrigo Siqueira
Cc: David Airlie, linux-kernel, dri-devel, Daniel Vetter,
virtualization
In-Reply-To: <20190311101159.t6mbzm5sdwuh5muc@smtp.gmail.com>
Hi,
> > > static void bochs_crtc_atomic_flush(struct drm_crtc *crtc,
> > > struct drm_crtc_state *old_crtc_state)
> > > {
> > > @@ -66,6 +71,7 @@ static const struct drm_crtc_funcs bochs_crtc_funcs = {
> > > static const struct drm_crtc_helper_funcs bochs_helper_funcs = {
> > > .mode_set_nofb = bochs_crtc_mode_set_nofb,
> > > .atomic_enable = bochs_crtc_atomic_enable,
> > > + .atomic_disable = bochs_crtc_atomic_disable,
> >
> > Shouldn't we make the callback optional instead of adding empty dummy
> > functions to drivers?
>
> Hi Gerd,
>
> I agree, and I can work in this issue.
> Just one question, should we make atomic_enable optional as well?
IIRC the drm code checks for the atomic_enable callback presence to
figure whenever it should take the atomic or legacy code paths.
So, I think that will not work.
cheers,
Gerd
^ permalink raw reply
* Re: [PATCH] drm/bochs: Fix NULL dereference on atomic_disable helper
From: Daniel Vetter @ 2019-03-11 13:23 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: Rodrigo Siqueira, David Airlie, linux-kernel, dri-devel,
virtualization, Daniel Vetter
In-Reply-To: <20190311130716.6hjqci43clic5lxx@sirius.home.kraxel.org>
On Mon, Mar 11, 2019 at 02:07:16PM +0100, Gerd Hoffmann wrote:
> Hi,
>
> > > > static void bochs_crtc_atomic_flush(struct drm_crtc *crtc,
> > > > struct drm_crtc_state *old_crtc_state)
> > > > {
> > > > @@ -66,6 +71,7 @@ static const struct drm_crtc_funcs bochs_crtc_funcs = {
> > > > static const struct drm_crtc_helper_funcs bochs_helper_funcs = {
> > > > .mode_set_nofb = bochs_crtc_mode_set_nofb,
> > > > .atomic_enable = bochs_crtc_atomic_enable,
> > > > + .atomic_disable = bochs_crtc_atomic_disable,
> > >
> > > Shouldn't we make the callback optional instead of adding empty dummy
> > > functions to drivers?
> >
> > Hi Gerd,
> >
> > I agree, and I can work in this issue.
> > Just one question, should we make atomic_enable optional as well?
>
> IIRC the drm code checks for the atomic_enable callback presence to
> figure whenever it should take the atomic or legacy code paths.
It should check for drm_driver->mode_config.funcs.atomic_commit for that,
see drm_drv_uses_atomic_modeset(). Anything else should be a bug.
Or do you mean the fallback to the old crtc helper prepare/commit
callbacks? We'd need to make all of them optional ofc, with atomic_
variants being preferred ofc.
-Daniel
>
> So, I think that will not work.
>
> cheers,
> Gerd
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply
* Re: [PATCH] drm/bochs: Fix NULL dereference on atomic_disable helper
From: Gerd Hoffmann @ 2019-03-11 13:39 UTC (permalink / raw)
To: Rodrigo Siqueira, David Airlie, virtualization, dri-devel,
linux-kernel
In-Reply-To: <20190311132358.GF2665@phenom.ffwll.local>
Hi,
> > IIRC the drm code checks for the atomic_enable callback presence to
> > figure whenever it should take the atomic or legacy code paths.
>
> It should check for drm_driver->mode_config.funcs.atomic_commit for that,
> see drm_drv_uses_atomic_modeset(). Anything else should be a bug.
>
> Or do you mean the fallback to the old crtc helper prepare/commit
> callbacks?
Probably the later. There was some reason why I've left in the empty
bochs_crtc_atomic_enable() callback ...
cheers,
Gerd
^ permalink raw reply
* Re: [RFC PATCH V2 5/5] vhost: access vq metadata through kernel virtual address
From: Andrea Arcangeli @ 2019-03-11 13:43 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: kvm, netdev, linux-kernel, virtualization, linux-mm,
Jerome Glisse
In-Reply-To: <20190311084525-mutt-send-email-mst@kernel.org>
On Mon, Mar 11, 2019 at 08:48:37AM -0400, Michael S. Tsirkin wrote:
> Using copyXuser is better I guess.
It certainly would be faster there, but I don't think it's needed if
that would be the only use case left that justifies supporting two
different models. On small 32bit systems with little RAM kmap won't
perform measurably different on 32bit or 64bit systems. If the 32bit
host has a lot of ram it all gets slow anyway at accessing RAM above
the direct mapping, if compared to 64bit host kernels, it's not just
an issue for vhost + mmu notifier + kmap and the best way to optimize
things is to run 64bit host kernels.
Like Christoph pointed out, the main use case for retaining the
copy-user model would be CPUs with virtually indexed not physically
tagged data caches (they'll still suffer from the spectre-v1 fix,
although I exclude they have to suffer the SMAP
slowdown/feature). Those may require some additional flushing than the
current copy-user model requires.
As a rule of thumb any arch where copy_user_page doesn't define as
copy_page will require some additional cache flushing after the
kmap. Supposedly with vmap, the vmap layer should have taken care of
that (I didn't verify that yet).
There are some accessories like copy_to_user_page()
copy_from_user_page() that could work and obviously defines to raw
memcpy on x86 (the main cons is they don't provide word granular
access) and at least on sparc they're tailored to ptrace assumptions
so then we'd need to evaluate what happens if this is used outside of
ptrace context. kmap has been used generally either to access whole
pages (i.e. copy_user_page), so ptrace may actually be the only use
case with subpage granularity access.
#define copy_to_user_page(vma, page, vaddr, dst, src, len) \
do { \
flush_cache_page(vma, vaddr, page_to_pfn(page)); \
memcpy(dst, src, len); \
flush_ptrace_access(vma, page, vaddr, src, len, 0); \
} while (0)
So I wouldn't rule out the need for a dual model, until we solve how
to run this stable on non-x86 arches with not physically tagged
caches.
Thanks,
Andrea
^ permalink raw reply
* Re: [RFC PATCH V2 0/5] vhost: accelerate metadata access through vmap()
From: Michael S. Tsirkin @ 2019-03-11 13:59 UTC (permalink / raw)
To: Jason Wang
Cc: aarcange, linux-parisc, kvm, netdev, linux-kernel, virtualization,
Christoph Hellwig, linux-mm, linux-arm-kernel
In-Reply-To: <56374231-7ba7-0227-8d6d-4d968d71b4d6@redhat.com>
On Mon, Mar 11, 2019 at 03:13:17PM +0800, Jason Wang wrote:
>
> On 2019/3/8 下午10:12, Christoph Hellwig wrote:
> > On Wed, Mar 06, 2019 at 02:18:07AM -0500, Jason Wang wrote:
> > > This series tries to access virtqueue metadata through kernel virtual
> > > address instead of copy_user() friends since they had too much
> > > overheads like checks, spec barriers or even hardware feature
> > > toggling. This is done through setup kernel address through vmap() and
> > > resigter MMU notifier for invalidation.
> > >
> > > Test shows about 24% improvement on TX PPS. TCP_STREAM doesn't see
> > > obvious improvement.
> > How is this going to work for CPUs with virtually tagged caches?
>
>
> Anything different that you worry?
If caches have virtual tags then kernel and userspace view of memory
might not be automatically in sync if they access memory
through different virtual addresses. You need to do things like
flush_cache_page, probably multiple times.
> I can have a test but do you know any
> archs that use virtual tag cache?
sparc I believe.
> Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC PATCH V2 5/5] vhost: access vq metadata through kernel virtual address
From: Jan Kara @ 2019-03-11 14:45 UTC (permalink / raw)
To: Andrea Arcangeli
Cc: Jan Kara, kvm, Michael S. Tsirkin, netdev, linux-kernel,
virtualization, linux-mm, Jerome Glisse
In-Reply-To: <20190307212717.GS23850@redhat.com>
On Thu 07-03-19 16:27:17, Andrea Arcangeli wrote:
> > driver that GUP page for hours/days/weeks/months ... obviously the
> > race window is big enough here. It affects many fs (ext4, xfs, ...)
> > in different ways. I think ext4 is the most obvious because of the
> > kernel log trace it leaves behind.
> >
> > Bottom line is for set_page_dirty to be safe you need the following:
> > lock_page()
> > page_mkwrite()
> > set_pte_with_write()
> > unlock_page()
>
> I also wondered why ext4 writepage doesn't recreate the bh if they got
> dropped by the VM and page->private is 0. I mean, page->index and
> page->mapping are still there, that's enough info for writepage itself
> to take a slow path and calls page_mkwrite to find where to write the
> page on disk.
There are two problems:
1) What to do with errors that page_mkwrite() can generate (ENOMEM, ENOSPC,
EIO). On page fault you just propagate them to userspace, on set_page_dirty()
you have no chance so you just silently loose data.
2) We need various locks to protect page_mkwrite(), possibly do some IO.
set_page_dirty() is rather uncertain context to acquire locks or do IO...
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* Re: [PATCH 0/5] Clean up TTM mmap offsets
From: Hans de Goede @ 2019-03-11 16:39 UTC (permalink / raw)
To: Thomas Zimmermann, alexander.deucher, christian.koenig,
David1.Zhou, airlied, kraxel, z.liuxinliang, zourongrong,
kong.kongxinwei, puck.chen, bskeggs, thellstrom,
linux-graphics-maintainer, daniel, arnd, gregkh, ray.huang,
Jerry.Zhang
Cc: spice-devel, amd-gfx, dri-devel, virtualization
In-Reply-To: <20190207085931.29569-1-tzimmermann@suse.de>
Hi,
On 07-02-19 09:59, Thomas Zimmermann wrote:
> Almost all TTM-based drivers use the same values for the mmap-able
> range of BO addresses. Each driver therefore duplicates the
> DRM_FILE_PAGE_OFFSET constant. OTOH, the mmap range's size is not
> configurable by drivers.
>
> This patch set replaces driver-specific configuration with a single
> setup. All code is located within TTM. TTM and GEM share the same
> range for mmap-able BOs.
>
> Thomas Zimmermann (5):
> staging/vboxvideo: Use same BO mmap offset as other drivers
> drm/ttm: Define a single DRM_FILE_PAGE_OFFSET constant
> drm/ttm: Remove file_page_offset parameter from ttm_bo_device_init()
> drm/ttm: Quick-test mmap offset in ttm_bo_mmap()
> drm: Use the same mmap-range offset and size for GEM and TTM
Note I'm about to push a patch-series to drm-misc-next which moves
vboxvideo out of staging and I see that this series has not landed
in drm-misc-next yet, so it will needs to be rebased.
Regards,
Hans
^ permalink raw reply
* Re: [PATCH 0/5] Clean up TTM mmap offsets
From: Hans de Goede @ 2019-03-11 17:00 UTC (permalink / raw)
To: christian.koenig, Thomas Zimmermann, alexander.deucher,
David1.Zhou, airlied, kraxel, z.liuxinliang, zourongrong,
kong.kongxinwei, puck.chen, bskeggs, thellstrom,
linux-graphics-maintainer, daniel, arnd, gregkh, ray.huang,
Jerry.Zhang
Cc: spice-devel, dri-devel, amd-gfx, virtualization
In-Reply-To: <0ac91c9b-5b8e-b27c-286b-7b9697a4f1c2@gmail.com>
Hi,
On 11-03-19 17:51, Christian König wrote:
> Am 11.03.19 um 17:39 schrieb Hans de Goede:
>> Hi,
>>
>> On 07-02-19 09:59, Thomas Zimmermann wrote:
>>> Almost all TTM-based drivers use the same values for the mmap-able
>>> range of BO addresses. Each driver therefore duplicates the
>>> DRM_FILE_PAGE_OFFSET constant. OTOH, the mmap range's size is not
>>> configurable by drivers.
>>>
>>> This patch set replaces driver-specific configuration with a single
>>> setup. All code is located within TTM. TTM and GEM share the same
>>> range for mmap-able BOs.
>>>
>>> Thomas Zimmermann (5):
>>> staging/vboxvideo: Use same BO mmap offset as other drivers
>>> drm/ttm: Define a single DRM_FILE_PAGE_OFFSET constant
>>> drm/ttm: Remove file_page_offset parameter from ttm_bo_device_init()
>>> drm/ttm: Quick-test mmap offset in ttm_bo_mmap()
>>> drm: Use the same mmap-range offset and size for GEM and TTM
>>
>> Note I'm about to push a patch-series to drm-misc-next which moves
>> vboxvideo out of staging and I see that this series has not landed
>> in drm-misc-next yet, so it will needs to be rebased.
>
> Mhm, TTM is usually not pushed upstream through drm-misc-next, so that will certainly collide with the next TTM pull request.
Ugh, I didn't realize that this series would not be going through drm-misc-next.
> So can you wait with that or should I make an exception and merge this change though drm-misc-next?
I've already pushed it now :| My mail was more intended as a headsup then
that I expected an objection, sorry.
I see 2 possible solutions:
1) Merge drm-misc-next into the ttm tree (probably the cleanest)
2) Push your series through drm-misc-next
Regards,
Hans
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH 0/5] Clean up TTM mmap offsets
From: Daniel Vetter @ 2019-03-11 17:02 UTC (permalink / raw)
To: christian.koenig
Cc: dri-devel, virtualization, ray.huang, alexander.deucher,
David1.Zhou, thellstrom, amd-gfx, z.liuxinliang, kong.kongxinwei,
linux-graphics-maintainer, bskeggs, spice-devel,
Thomas Zimmermann, arnd, puck.chen, Hans de Goede, airlied,
Jerry.Zhang, gregkh, daniel, zourongrong
In-Reply-To: <0ac91c9b-5b8e-b27c-286b-7b9697a4f1c2@gmail.com>
On Mon, Mar 11, 2019 at 05:51:39PM +0100, Christian König wrote:
> Am 11.03.19 um 17:39 schrieb Hans de Goede:
> > Hi,
> >
> > On 07-02-19 09:59, Thomas Zimmermann wrote:
> > > Almost all TTM-based drivers use the same values for the mmap-able
> > > range of BO addresses. Each driver therefore duplicates the
> > > DRM_FILE_PAGE_OFFSET constant. OTOH, the mmap range's size is not
> > > configurable by drivers.
> > >
> > > This patch set replaces driver-specific configuration with a single
> > > setup. All code is located within TTM. TTM and GEM share the same
> > > range for mmap-able BOs.
> > >
> > > Thomas Zimmermann (5):
> > > staging/vboxvideo: Use same BO mmap offset as other drivers
> > > drm/ttm: Define a single DRM_FILE_PAGE_OFFSET constant
> > > drm/ttm: Remove file_page_offset parameter from ttm_bo_device_init()
> > > drm/ttm: Quick-test mmap offset in ttm_bo_mmap()
> > > drm: Use the same mmap-range offset and size for GEM and TTM
> >
> > Note I'm about to push a patch-series to drm-misc-next which moves
> > vboxvideo out of staging and I see that this series has not landed
> > in drm-misc-next yet, so it will needs to be rebased.
>
> Mhm, TTM is usually not pushed upstream through drm-misc-next, so that will
> certainly collide with the next TTM pull request.
>
> So can you wait with that or should I make an exception and merge this
> change though drm-misc-next?
Other options:
- Get amdgpu added to drm-tip and linux-next so we have a heads-up about
the conflict. That's usually good enough to avoid the broken merge
conflict.
- Do a topic branch, pull it into both trees.
- Really stuff ttm into a shared tree if it's meant to be shared
infrastructure :-P
Waiting+rebasing is imo the worst option, and usually not needed.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply
* Re: [RFC PATCH V2 0/5] vhost: accelerate metadata access through vmap()
From: David Miller @ 2019-03-11 18:14 UTC (permalink / raw)
To: mst
Cc: aarcange, linux-parisc, kvm, netdev, linux-kernel, virtualization,
hch, linux-mm, linux-arm-kernel
In-Reply-To: <20190311095405-mutt-send-email-mst@kernel.org>
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Mon, 11 Mar 2019 09:59:28 -0400
> On Mon, Mar 11, 2019 at 03:13:17PM +0800, Jason Wang wrote:
>>
>> On 2019/3/8 下午10:12, Christoph Hellwig wrote:
>> > On Wed, Mar 06, 2019 at 02:18:07AM -0500, Jason Wang wrote:
>> > > This series tries to access virtqueue metadata through kernel virtual
>> > > address instead of copy_user() friends since they had too much
>> > > overheads like checks, spec barriers or even hardware feature
>> > > toggling. This is done through setup kernel address through vmap() and
>> > > resigter MMU notifier for invalidation.
>> > >
>> > > Test shows about 24% improvement on TX PPS. TCP_STREAM doesn't see
>> > > obvious improvement.
>> > How is this going to work for CPUs with virtually tagged caches?
>>
>>
>> Anything different that you worry?
>
> If caches have virtual tags then kernel and userspace view of memory
> might not be automatically in sync if they access memory
> through different virtual addresses. You need to do things like
> flush_cache_page, probably multiple times.
"flush_dcache_page()"
^ permalink raw reply
* Re: [PATCH] drm/bochs: Fix NULL dereference on atomic_disable helper
From: Daniel Vetter @ 2019-03-11 18:48 UTC (permalink / raw)
To: Rodrigo Siqueira; +Cc: David Airlie, virtualization, dri-devel, linux-kernel
In-Reply-To: <20190311174958.xdw4lrvf6ydeplvc@smtp.gmail.com>
On Mon, Mar 11, 2019 at 02:49:58PM -0300, Rodrigo Siqueira wrote:
> On 03/11, Gerd Hoffmann wrote:
> > Hi,
> >
> > > > IIRC the drm code checks for the atomic_enable callback presence to
> > > > figure whenever it should take the atomic or legacy code paths.
> > >
> > > It should check for drm_driver->mode_config.funcs.atomic_commit for that,
> > > see drm_drv_uses_atomic_modeset(). Anything else should be a bug.
> > >
> > > Or do you mean the fallback to the old crtc helper prepare/commit
> > > callbacks?
> >
> > Probably the later. There was some reason why I've left in the empty
> > bochs_crtc_atomic_enable() callback ...
>
> Just for checking before I start to work in this patch:
> The correct solution should be made atomic_enable and atomic_disable
> optional, right? I should do it, and check if Bochs driver really needs
> bochs_crtc_atomic_enable after my change, right?
Yup. I just tried to remember why we haven't done this yet, but I think
that was a patch to make crtc->helper_funcs optional. And that doesn't
make sense imo, since if your crtc doesn't do anything then you don't
really have an atomic driver :-) And if there's ever a legit use case for
this, then that drive probably shouldn't use the atomic helpers ...
But making crtc_helper_funcs->atomic_enable/disable optional sounds like a
good idea.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply
* Re: [PATCH] VMCI: Use BIT() macro for bit definitions
From: Randy Dunlap @ 2019-03-11 22:38 UTC (permalink / raw)
To: Vishnu DASA, gregkh@linuxfoundation.org,
linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org
Cc: Pv-drivers
In-Reply-To: <20190311220928.4560-1-vdasa@vmware.com>
On 3/11/19 3:09 PM, Vishnu DASA wrote:
> No functional changes, cleanup only.
>
> Reviewed-by: Adit Ranadive <aditr@vmware.com>
> Reviewed-by: Jorgen Hansen <jhansen@vmware.com>
> Signed-off-by: Vishnu Dasa <vdasa@vmware.com>
> ---
> include/linux/vmw_vmci_defs.h | 34 +++++++++++++++++-----------------
> 1 file changed, 17 insertions(+), 17 deletions(-)
>
Now this header file needs to #include <linux/bitops.h>
or <linux/bits.h> for the BIT() macro.
Do the users of this header file build cleanly anyway?
Even if so, we prefer not to depend on implicit or accidental header
file inclusions that may change in the future.
> diff --git a/include/linux/vmw_vmci_defs.h b/include/linux/vmw_vmci_defs.h
> index eaa1e762bf06..007e28053da8 100644
> --- a/include/linux/vmw_vmci_defs.h
> +++ b/include/linux/vmw_vmci_defs.h
> @@ -33,27 +33,27 @@
> #define VMCI_MAX_DEVICES 1
>
> /* Status register bits. */
> -#define VMCI_STATUS_INT_ON 0x1
> +#define VMCI_STATUS_INT_ON BIT(0)
>
> /* Control register bits. */
> -#define VMCI_CONTROL_RESET 0x1
> -#define VMCI_CONTROL_INT_ENABLE 0x2
> -#define VMCI_CONTROL_INT_DISABLE 0x4
> +#define VMCI_CONTROL_RESET BIT(0)
> +#define VMCI_CONTROL_INT_ENABLE BIT(1)
> +#define VMCI_CONTROL_INT_DISABLE BIT(2)
>
> /* Capabilities register bits. */
> -#define VMCI_CAPS_HYPERCALL 0x1
> -#define VMCI_CAPS_GUESTCALL 0x2
> -#define VMCI_CAPS_DATAGRAM 0x4
> -#define VMCI_CAPS_NOTIFICATIONS 0x8
> -#define VMCI_CAPS_PPN64 0x10
> +#define VMCI_CAPS_HYPERCALL BIT(0)
> +#define VMCI_CAPS_GUESTCALL BIT(1)
> +#define VMCI_CAPS_DATAGRAM BIT(2)
> +#define VMCI_CAPS_NOTIFICATIONS BIT(3)
> +#define VMCI_CAPS_PPN64 BIT(4)
>
> /* Interrupt Cause register bits. */
> -#define VMCI_ICR_DATAGRAM 0x1
> -#define VMCI_ICR_NOTIFICATION 0x2
> +#define VMCI_ICR_DATAGRAM BIT(0)
> +#define VMCI_ICR_NOTIFICATION BIT(1)
>
> /* Interrupt Mask register bits. */
> -#define VMCI_IMR_DATAGRAM 0x1
> -#define VMCI_IMR_NOTIFICATION 0x2
> +#define VMCI_IMR_DATAGRAM BIT(0)
> +#define VMCI_IMR_NOTIFICATION BIT(1)
>
> /* Maximum MSI/MSI-X interrupt vectors in the device. */
> #define VMCI_MAX_INTRS 2
> @@ -463,9 +463,9 @@ struct vmci_datagram {
> * datagram callback is invoked in a delayed context (not interrupt context).
> */
> #define VMCI_FLAG_DG_NONE 0
> -#define VMCI_FLAG_WELLKNOWN_DG_HND 0x1
> -#define VMCI_FLAG_ANYCID_DG_HND 0x2
> -#define VMCI_FLAG_DG_DELAYED_CB 0x4
> +#define VMCI_FLAG_WELLKNOWN_DG_HND BIT(0)
> +#define VMCI_FLAG_ANYCID_DG_HND BIT(1)
> +#define VMCI_FLAG_DG_DELAYED_CB BIT(2)
>
> /*
> * Maximum supported size of a VMCI datagram for routable datagrams.
> @@ -694,7 +694,7 @@ struct vmci_qp_detach_msg {
> };
>
> /* VMCI Doorbell API. */
> -#define VMCI_FLAG_DELAYED_CB 0x01
> +#define VMCI_FLAG_DELAYED_CB BIT(0)
>
> typedef void (*vmci_callback) (void *client_data);
>
>
--
~Randy
^ permalink raw reply
* [PATCH 1/1] virtio_blk: replace 0 by HCTX_TYPE_DEFAULT to index blk_mq_tag_set->map
From: Dongli Zhang @ 2019-03-12 1:31 UTC (permalink / raw)
To: virtualization, linux-block, linux-kernel; +Cc: axboe, mst
Use HCTX_TYPE_DEFAULT instead of 0 to avoid hardcoding.
Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
---
This is the only place to cleanup under drivers/block/*. Another patch set
is sent to scsi and then all of below are cleaned up:
- block/*
- drivers/*
drivers/block/virtio_blk.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 4bc083b..bed6035 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -691,7 +691,8 @@ static int virtblk_map_queues(struct blk_mq_tag_set *set)
{
struct virtio_blk *vblk = set->driver_data;
- return blk_mq_virtio_map_queues(&set->map[0], vblk->vdev, 0);
+ return blk_mq_virtio_map_queues(&set->map[HCTX_TYPE_DEFAULT],
+ vblk->vdev, 0);
}
#ifdef CONFIG_VIRTIO_BLK_SCSI
--
2.7.4
^ permalink raw reply related
* Re: [RFC PATCH V2 5/5] vhost: access vq metadata through kernel virtual address
From: Jason Wang @ 2019-03-12 2:52 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Andrea Arcangeli, kvm, netdev, linux-kernel, virtualization,
linux-mm, Jerome Glisse
In-Reply-To: <20190311084525-mutt-send-email-mst@kernel.org>
On 2019/3/11 下午8:48, Michael S. Tsirkin wrote:
> On Mon, Mar 11, 2019 at 03:40:31PM +0800, Jason Wang wrote:
>> On 2019/3/9 上午3:48, Andrea Arcangeli wrote:
>>> Hello Jeson,
>>>
>>> On Fri, Mar 08, 2019 at 04:50:36PM +0800, Jason Wang wrote:
>>>> Just to make sure I understand here. For boosting through huge TLB, do
>>>> you mean we can do that in the future (e.g by mapping more userspace
>>>> pages to kenrel) or it can be done by this series (only about three 4K
>>>> pages were vmapped per virtqueue)?
>>> When I answered about the advantages of mmu notifier and I mentioned
>>> guaranteed 2m/gigapages where available, I overlooked the detail you
>>> were using vmap instead of kmap. So with vmap you're actually doing
>>> the opposite, it slows down the access because it will always use a 4k
>>> TLB even if QEMU runs on THP or gigapages hugetlbfs.
>>>
>>> If there's just one page (or a few pages) in each vmap there's no need
>>> of vmap, the linearity vmap provides doesn't pay off in such
>>> case.
>>>
>>> So likely there's further room for improvement here that you can
>>> achieve in the current series by just dropping vmap/vunmap.
>>>
>>> You can just use kmap (or kmap_atomic if you're in preemptible
>>> section, should work from bh/irq).
>>>
>>> In short the mmu notifier to invalidate only sets a "struct page *
>>> userringpage" pointer to NULL without calls to vunmap.
>>>
>>> In all cases immediately after gup_fast returns you can always call
>>> put_page immediately (which explains why I'd like an option to drop
>>> FOLL_GET from gup_fast to speed it up).
>>>
>>> Then you can check the sequence_counter and inc/dec counter increased
>>> by _start/_end. That will tell you if the page you got and you called
>>> put_page to immediately unpin it or even to free it, cannot go away
>>> under you until the invalidate is called.
>>>
>>> If sequence counters and counter tells that gup_fast raced with anyt
>>> mmu notifier invalidate you can just repeat gup_fast. Otherwise you're
>>> done, the page cannot go away under you, the host virtual to host
>>> physical mapping cannot change either. And the page is not pinned
>>> either. So you can just set the "struct page * userringpage = page"
>>> where "page" was the one setup by gup_fast.
>>>
>>> When later the invalidate runs, you can just call set_page_dirty if
>>> gup_fast was called with "write = 1" and then you clear the pointer
>>> "userringpage = NULL".
>>>
>>> When you need to read/write to the memory
>>> kmap/kmap_atomic(userringpage) should work.
>> Yes, I've considered kmap() from the start. The reason I don't do that is
>> large virtqueue may need more than one page so VA might not be contiguous.
>> But this is probably not a big issue which just need more tricks in the
>> vhost memory accessors.
>>
>>
>>> In short because there's no hardware involvement here, the established
>>> mapping is just the pointer to the page, there is no need of setting
>>> up any pagetables or to do any TLB flushes (except on 32bit archs if
>>> the page is above the direct mapping but it never happens on 64bit
>>> archs).
>> I see, I believe we don't care much about the performance of 32bit archs (or
>> we can just fallback to copy_to_user() friends).
> Using copyXuser is better I guess.
Ok.
>
>> Using direct mapping (I
>> guess kernel will always try hugepage for that?) should be better and we can
>> even use it for the data transfer not only for the metadata.
>>
>> Thanks
> We can't really. The big issue is get user pages. Doing that on data
> path will be slower than copyXuser.
I meant if we can find a way to avoid doing gup in datapath. E.g vhost
maintain a range tree and add or remove ranges through MMU notifier.
Then in datapath, if we find the range, then use direct mapping
otherwise copy_to_user().
Thanks
> Or maybe it won't with the
> amount of mitigations spread around. Go ahead and try.
>
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox