Date:   Thu, 2 Mar 2023 16:30:04 +0100
From:   Simon Horman <simon.horman@corigine.com>
To:     Stefano Garzarella <sgarzare@redhat.com>
Cc:     virtualization@lists.linux-foundation.org,
        Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>,
        eperezma@redhat.com, netdev@vger.kernel.org, stefanha@redhat.com,
        linux-kernel@vger.kernel.org, Jason Wang <jasowang@redhat.com>,
        "Michael S. Tsirkin" <mst@redhat.com>, kvm@vger.kernel.org
Subject: Re: [PATCH v2 6/8] vdpa_sim: use kthread worker
Message-ID: <ZADA/GgpbDoi+SzU@corigine.com>
References: <20230302113421.174582-1-sgarzare@redhat.com>
 <20230302113421.174582-7-sgarzare@redhat.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230302113421.174582-7-sgarzare@redhat.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?6Ur9Wn+TxdwreYHXGJJtL0nSMfejlU1O7iVf98wjzoaFRhjBWL2WayV7wkHw?=
 =?us-ascii?Q?O8lojtLuZCXXfzPBwxS6FOF3bMFh1py20ZEBZPFj3AT4+lHlHHtMxIdjY0rG?=
 =?us-ascii?Q?03QtET/pseb5B/yIHF9pptuzLjwNi5qgXJobeWj4Pt1tt9mYnIQskECnL3DK?=
 =?us-ascii?Q?XrWUQ88AzhjMRnmJX2PyKYN7THuvq8EhUo7MPFI6I+K5InBYaC/K3a1vcWLl?=
 =?us-ascii?Q?OQ3EeS+QMz5bS7vDhJtMLsm8cIG0fnnSRWaMFhOITRcRvhm2z2yv5EtEaaGR?=
 =?us-ascii?Q?TfGTdeEZAJB7Pv6yyOUalO2w7iA/WvWHzKXjlWhOyZN6++Dwc6t7yUNL+WRU?=
 =?us-ascii?Q?/oJlaiCqWLXqwLg02QJQzGOJOOL37MTue0/v9zAHeDtSuM/GQan8NM1BC8w6?=
 =?us-ascii?Q?K70+G+LH6QGjBSmzmTCucD5iVxaQf8V5xyb5PsWrY5HqMmYijOYw/bpz7/YM?=
 =?us-ascii?Q?UUcysymOK3NHjzuJltAlyaZl690Zsjf4zfskpA5y72jaxE+SAeVsfam8jWN/?=
 =?us-ascii?Q?19qrsyhoy7TO0Zh9mEP/jmHI5wlc0xqAbo+xJrwVDsh1pwRXOGm+mib4w66A?=
 =?us-ascii?Q?RhTZT46AmLTd/ZfplY/Hp+3UNLWTUViBLEVk5ACI6bbE32i4ju0C17G0/a3i?=
 =?us-ascii?Q?CJhqBXr8HTgbcM/Gr+MNSOpCriueMO/zuaw1/MHqVAer+lLLNsyXN6pnbOH8?=
 =?us-ascii?Q?M2uQ0RlBovrrcY2Sqo5J1Cr6SRdAyh7FvXsrKgwWj7lZ/dkmmbO12C/Sziv5?=
 =?us-ascii?Q?S3BT5DqH+wV98ftl0qQfXDfiYrGWdHQPSZOXqXv0+XWi2NeXeauNhXDesQcs?=
 =?us-ascii?Q?nSzyuDzjNacGAPvCNxN/6nS6lDPK+seTxxZpUYxfuz9q7k5Uf/Qk6a39uvEA?=
 =?us-ascii?Q?Zlxd+X22krtFo3bJn6o9S1HlldZG6uCP2NTjLSEwZ4OltXtJj8g+cf0Bruyv?=
 =?us-ascii?Q?ZHfDH9fDzIyy9oESHmV4AnIgh3TehieXZrBOLJPZvtkzGJgQT90dmc6tZAxb?=
 =?us-ascii?Q?jxRVbJv4L9HDz4QEEqDYt49HkmcUyPzRvdcDhdfsoS2KofGxHBVbDHzNXez0?=
 =?us-ascii?Q?8DT7+w9ypCkptxBOmYjVYKgbeGJGR0642KOZkm8OhBoOahENMu24Dgnrswk+?=
 =?us-ascii?Q?YuzciBTpd7WryX5Iryr9mtbmTn/z1Ci24YSPZC47Oz4PN4yABk+P/VJKHnOv?=
 =?us-ascii?Q?2tt1an4ZqFxPjQ7yS/XD2yKhoWKucNC3G7vPqlwuEoO5RnpYQg6Pe9AAyf7s?=
 =?us-ascii?Q?WVppGFre3rjFmw/VoTNRRowttkHXckl/4/AtpUMkpSgG3WIYNO3LtflBVVpP?=
 =?us-ascii?Q?yewSVWWB3yLNdq6Y+8EK98xRL1xhJ5/7nhdPkK1f11wQohF/xw8JodvdHiC9?=
 =?us-ascii?Q?aHA1pHsDD4UzoqVayrb+hJhGzQLjqgveW/iQQ4vQOB8tKAr3Xv3UsifqE4Ed?=
 =?us-ascii?Q?mmNwbaszEx+cXF8ZQpsaLdQ0HflO/hxN7gjngCZ9avK6n5JpoQXV/C7XUn+v?=
 =?us-ascii?Q?IcV2OoNC0Jn0xY3z4YkhjCcsi4rJUOmEJhWs+oSEfUqvjUGDPq6SNeP2DiAf?=
 =?us-ascii?Q?1bHfwxeKfqpVy0TKh7bUM3QlVi1Or8+RTncQsG5TGaWd/odH/h+kAkNzf4h3?=
 =?us-ascii?Q?yd4lHSVo2PKpyarqABx++OPago8Gsan8kOaabRyd8+lN1zrIpjo54/VK+X2n?=
 =?us-ascii?Q?3bHCmw=3D=3D?=
X-OriginatorOrg: corigine.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 52d1f08e-4c44-41d0-4935-08db1b33031d
X-MS-Exchange-CrossTenant-AuthSource: PH0PR13MB4842.namprd13.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Mar 2023 15:30:11.4933
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: fe128f2c-073b-4c20-818e-7246a585940c
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: Zm5OieZJE/iKB72DUwPZterNqJ/Pw2SEMKWlu9c3JQ95d9olBOuOTH52c1sWbzWMFlC1ESYsHbH6pSFLKFSLblAScjY/b44pN+bQdLl3jTo=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH2PR13MB3832
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, Mar 02, 2023 at 12:34:19PM +0100, Stefano Garzarella wrote:
> Let's use our own kthread to run device jobs.
> This allows us more flexibility, especially we can attach the kthread
> to the user address space when vDPA uses user's VA.
> 
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
>  drivers/vdpa/vdpa_sim/vdpa_sim.h |  3 ++-
>  drivers/vdpa/vdpa_sim/vdpa_sim.c | 17 ++++++++++++-----
>  2 files changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.h b/drivers/vdpa/vdpa_sim/vdpa_sim.h
> index acee20faaf6a..ce83f9130a5d 100644
> --- a/drivers/vdpa/vdpa_sim/vdpa_sim.h
> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.h
> @@ -57,7 +57,8 @@ struct vdpasim_dev_attr {
>  struct vdpasim {
>  	struct vdpa_device vdpa;
>  	struct vdpasim_virtqueue *vqs;
> -	struct work_struct work;
> +	struct kthread_worker *worker;
> +	struct kthread_work work;
>  	struct vdpasim_dev_attr dev_attr;
>  	/* spinlock to synchronize virtqueue state */
>  	spinlock_t lock;
> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> index a6ee830efc38..6feb29726c2a 100644
> --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> @@ -11,8 +11,8 @@
>  #include <linux/module.h>
>  #include <linux/device.h>
>  #include <linux/kernel.h>
> +#include <linux/kthread.h>
>  #include <linux/slab.h>
> -#include <linux/sched.h>
>  #include <linux/dma-map-ops.h>
>  #include <linux/vringh.h>
>  #include <linux/vdpa.h>
> @@ -116,7 +116,7 @@ static void vdpasim_do_reset(struct vdpasim *vdpasim)
>  static const struct vdpa_config_ops vdpasim_config_ops;
>  static const struct vdpa_config_ops vdpasim_batch_config_ops;
>  
> -static void vdpasim_work_fn(struct work_struct *work)
> +static void vdpasim_work_fn(struct kthread_work *work)
>  {
>  	struct vdpasim *vdpasim = container_of(work, struct vdpasim, work);
>  
> @@ -159,7 +159,13 @@ struct vdpasim *vdpasim_create(struct vdpasim_dev_attr *dev_attr,
>  
>  	vdpasim = vdpa_to_sim(vdpa);
>  	vdpasim->dev_attr = *dev_attr;
> -	INIT_WORK(&vdpasim->work, vdpasim_work_fn);
> +
> +	kthread_init_work(&vdpasim->work, vdpasim_work_fn);
> +	vdpasim->worker = kthread_create_worker(0, "vDPA sim worker: %s",
> +						dev_attr->name);
> +	if (IS_ERR(vdpasim->worker))
> +		goto err_iommu;

Branching to err_iommu will result in a call to put_device(dev)...

> +
>  	spin_lock_init(&vdpasim->lock);
>  	spin_lock_init(&vdpasim->iommu_lock);

... but dev is not initialised until the line following this hunk,
which is:

	dev = &vdpasim->vdpa.dev;

In order to avoid leaking dev I _think_ the correct approach
is to move the initialisation of dev above the branch to
err_iommu, perhaps above the call to kthread_init_work()
is a good place.

This does move the assignment outside the locks above.
But I _think_ that is ok.

> @@ -212,7 +218,7 @@ EXPORT_SYMBOL_GPL(vdpasim_create);
>  
>  void vdpasim_schedule_work(struct vdpasim *vdpasim)
>  {
> -	schedule_work(&vdpasim->work);
> +	kthread_queue_work(vdpasim->worker, &vdpasim->work);
>  }
>  EXPORT_SYMBOL_GPL(vdpasim_schedule_work);
>  
> @@ -612,7 +618,8 @@ static void vdpasim_free(struct vdpa_device *vdpa)
>  	struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
>  	int i;
>  
> -	cancel_work_sync(&vdpasim->work);
> +	kthread_cancel_work_sync(&vdpasim->work);
> +	kthread_destroy_worker(vdpasim->worker);
>  
>  	for (i = 0; i < vdpasim->dev_attr.nvqs; i++) {
>  		vringh_kiov_cleanup(&vdpasim->vqs[i].out_iov);
> -- 
> 2.39.2
>