All of lore.kernel.org
 help / color / mirror / Atom feed
From: Stefano Garzarella <sgarzare@redhat.com>
To: Jason Wang <jasowang@redhat.com>
Cc: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>,
	kvm@vger.kernel.org, "Michael S. Tsirkin" <mst@redhat.com>,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	virtualization@lists.linux-foundation.org, eperezma@redhat.com,
	stefanha@redhat.com
Subject: Re: [PATCH v3 8/8] vdpa_sim: add support for user VA
Date: Thu, 23 Mar 2023 10:50:06 +0100	[thread overview]
Message-ID: <20230323095006.jvbbdjvkdvhzcehz@sgarzare-redhat> (raw)
In-Reply-To: <CACGkMEtbrt3zuqy9YdhNyE90HHUT1R=HF-YRAQ6b4KnW_SdZ-w@mail.gmail.com>

On Thu, Mar 23, 2023 at 11:42:07AM +0800, Jason Wang wrote:
>On Tue, Mar 21, 2023 at 11:48 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>>
>> The new "use_va" module parameter (default: true) is used in
>> vdpa_alloc_device() to inform the vDPA framework that the device
>> supports VA.
>>
>> vringh is initialized to use VA only when "use_va" is true and the
>> user's mm has been bound. So, only when the bus supports user VA
>> (e.g. vhost-vdpa).
>>
>> vdpasim_mm_work_fn work is used to serialize the binding to a new
>> address space when the .bind_mm callback is invoked, and unbinding
>> when the .unbind_mm callback is invoked.
>>
>> Call mmget_not_zero()/kthread_use_mm() inside the worker function
>> to pin the address space only as long as needed, following the
>> documentation of mmget() in include/linux/sched/mm.h:
>>
>>   * Never use this function to pin this address space for an
>>   * unbounded/indefinite amount of time.
>
>I wonder if everything would be simplified if we just allow the parent
>to advertise whether or not it requires the address space.
>
>Then when vhost-vDPA probes the device it can simply advertise
>use_work as true so vhost core can use get_task_mm() in this case?

IIUC set user_worker to true, it also creates the kthread in the vhost
core (but we can add another variable to avoid this).

My biggest concern is the comment in include/linux/sched/mm.h.
get_task_mm() uses mmget(), but in the documentation they advise against
pinning the address space indefinitely, so I preferred in keeping
mmgrab() in the vhost core, then call mmget_not_zero() in the worker
only when it is running.

In the future maybe mm will be used differently from parent if somehow
it is supported by iommu, so I would leave it to the parent to handle
this.

Thanks,
Stefano

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

WARNING: multiple messages have this Message-ID (diff)
From: Stefano Garzarella <sgarzare@redhat.com>
To: Jason Wang <jasowang@redhat.com>
Cc: virtualization@lists.linux-foundation.org, kvm@vger.kernel.org,
	stefanha@redhat.com, linux-kernel@vger.kernel.org,
	eperezma@redhat.com, "Michael S. Tsirkin" <mst@redhat.com>,
	Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>,
	netdev@vger.kernel.org
Subject: Re: [PATCH v3 8/8] vdpa_sim: add support for user VA
Date: Thu, 23 Mar 2023 10:50:06 +0100	[thread overview]
Message-ID: <20230323095006.jvbbdjvkdvhzcehz@sgarzare-redhat> (raw)
In-Reply-To: <CACGkMEtbrt3zuqy9YdhNyE90HHUT1R=HF-YRAQ6b4KnW_SdZ-w@mail.gmail.com>

On Thu, Mar 23, 2023 at 11:42:07AM +0800, Jason Wang wrote:
>On Tue, Mar 21, 2023 at 11:48 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>>
>> The new "use_va" module parameter (default: true) is used in
>> vdpa_alloc_device() to inform the vDPA framework that the device
>> supports VA.
>>
>> vringh is initialized to use VA only when "use_va" is true and the
>> user's mm has been bound. So, only when the bus supports user VA
>> (e.g. vhost-vdpa).
>>
>> vdpasim_mm_work_fn work is used to serialize the binding to a new
>> address space when the .bind_mm callback is invoked, and unbinding
>> when the .unbind_mm callback is invoked.
>>
>> Call mmget_not_zero()/kthread_use_mm() inside the worker function
>> to pin the address space only as long as needed, following the
>> documentation of mmget() in include/linux/sched/mm.h:
>>
>>   * Never use this function to pin this address space for an
>>   * unbounded/indefinite amount of time.
>
>I wonder if everything would be simplified if we just allow the parent
>to advertise whether or not it requires the address space.
>
>Then when vhost-vDPA probes the device it can simply advertise
>use_work as true so vhost core can use get_task_mm() in this case?

IIUC set user_worker to true, it also creates the kthread in the vhost
core (but we can add another variable to avoid this).

My biggest concern is the comment in include/linux/sched/mm.h.
get_task_mm() uses mmget(), but in the documentation they advise against
pinning the address space indefinitely, so I preferred in keeping
mmgrab() in the vhost core, then call mmget_not_zero() in the worker
only when it is running.

In the future maybe mm will be used differently from parent if somehow
it is supported by iommu, so I would leave it to the parent to handle
this.

Thanks,
Stefano


  reply	other threads:[~2023-03-23  9:50 UTC|newest]

Thread overview: 53+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-03-21 15:42 [PATCH v3 0/8] vdpa_sim: add support for user VA Stefano Garzarella
2023-03-21 15:42 ` Stefano Garzarella
2023-03-21 15:42 ` [PATCH v3 1/8] vdpa: add bind_mm/unbind_mm callbacks Stefano Garzarella
2023-03-21 15:42   ` Stefano Garzarella
2023-03-23  2:58   ` Jason Wang
2023-03-23  2:58     ` Jason Wang
2023-03-21 15:42 ` [PATCH v3 2/8] vhost-vdpa: use bind_mm/unbind_mm device callbacks Stefano Garzarella
2023-03-21 15:42   ` Stefano Garzarella
2023-03-23  3:01   ` Jason Wang
2023-03-23  3:01     ` Jason Wang
2023-03-23  9:38     ` Stefano Garzarella
2023-03-23  9:38       ` Stefano Garzarella
2023-03-21 15:42 ` [PATCH v3 3/8] vringh: replace kmap_atomic() with kmap_local_page() Stefano Garzarella
2023-03-21 15:42   ` Stefano Garzarella
2023-03-22 10:37   ` Fabio M. De Francesco
2023-03-23  3:02   ` Jason Wang
2023-03-23  3:02     ` Jason Wang
2023-03-21 15:42 ` [PATCH v3 4/8] vringh: support VA with iotlb Stefano Garzarella
2023-03-21 15:42   ` Stefano Garzarella
2023-03-23  3:36   ` Jason Wang
2023-03-23  3:36     ` Jason Wang
2023-03-23 10:37     ` Stefano Garzarella
2023-03-23 10:37       ` Stefano Garzarella
2023-03-23  8:09   ` Eugenio Perez Martin
2023-03-23 10:46     ` Stefano Garzarella
2023-03-23 10:46       ` Stefano Garzarella
2023-03-23 14:43       ` Eugenio Perez Martin
2023-03-24 14:39         ` Stefano Garzarella
2023-03-24 14:39           ` Stefano Garzarella
2023-03-21 15:48 ` [PATCH v3 5/8] vdpa_sim: make devices agnostic for work management Stefano Garzarella
2023-03-21 15:48   ` Stefano Garzarella
2023-03-21 15:48   ` [PATCH v3 6/8] vdpa_sim: use kthread worker Stefano Garzarella
2023-03-21 15:48     ` Stefano Garzarella
2023-03-21 15:48   ` [PATCH v3 7/8] vdpa_sim: replace the spinlock with a mutex to protect the state Stefano Garzarella
2023-03-21 15:48     ` Stefano Garzarella
2023-03-21 15:48   ` [PATCH v3 8/8] vdpa_sim: add support for user VA Stefano Garzarella
2023-03-21 15:48     ` Stefano Garzarella
2023-03-23  3:42     ` Jason Wang
2023-03-23  3:42       ` Jason Wang
2023-03-23  9:50       ` Stefano Garzarella [this message]
2023-03-23  9:50         ` Stefano Garzarella
2023-03-23 11:44         ` Michael S. Tsirkin
2023-03-23 11:44           ` Michael S. Tsirkin
2023-03-24  2:54         ` Jason Wang
2023-03-24  2:54           ` Jason Wang
2023-03-24 14:43           ` Stefano Garzarella
2023-03-24 14:43             ` Stefano Garzarella
2023-03-27  3:12             ` Jason Wang
2023-03-27  3:12               ` Jason Wang
2023-03-24  3:49     ` Jason Wang
2023-03-24  3:49       ` Jason Wang
2023-03-24 14:46       ` Stefano Garzarella
2023-03-24 14:46         ` Stefano Garzarella

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=20230323095006.jvbbdjvkdvhzcehz@sgarzare-redhat \
    --to=sgarzare@redhat.com \
    --cc=andrey.zhadchenko@virtuozzo.com \
    --cc=eperezma@redhat.com \
    --cc=jasowang@redhat.com \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mst@redhat.com \
    --cc=netdev@vger.kernel.org \
    --cc=stefanha@redhat.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: link
Be 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.