Received-SPF: pass (google.com: domain of linux-kernel+bounces-65898-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <996cfabf-c612-43c8-9e56-03fb58a1ebec@oracle.com>
Date: Wed, 14 Feb 2024 14:52:17 -0500
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH V2 3/3] vdpa_sim: flush workers on suspend
Content-Language: en-US
To: Eugenio Perez Martin <eperezma@redhat.com>
Cc: virtualization@lists.linux-foundation.org, linux-kernel@vger.kernel.org,
        "Michael S. Tsirkin" <mst@redhat.com>,
        Jason Wang <jasowang@redhat.com>, Si-Wei Liu <si-wei.liu@oracle.com>,
        Xie Yongji <xieyongji@bytedance.com>,
        Stefano Garzarella <sgarzare@redhat.com>
References: <1707758174-142161-1-git-send-email-steven.sistare@oracle.com>
 <1707758174-142161-4-git-send-email-steven.sistare@oracle.com>
 <CAJaqyWfYHqf2=8BMo5ReKEB137fxGZR4XEJ2d4imXOOXAX2wHQ@mail.gmail.com>
 <e1b80fee-30df-4733-9072-ce67e3edc72f@oracle.com>
 <CAJaqyWe1=eQ4eQyv+wOqbr3pfZst6gmss2SrDZSNkY_ZVXcf=Q@mail.gmail.com>
From: Steven Sistare <steven.sistare@oracle.com>
Organization: Oracle Corporation
In-Reply-To: <CAJaqyWe1=eQ4eQyv+wOqbr3pfZst6gmss2SrDZSNkY_ZVXcf=Q@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?utf-8?B?eVp6Vys4eWdtc3pkQ1VIUUxjTU4vMVg0cjk2cE5CSUVCb3Y2aHhwZXlXTTMy?=
 =?utf-8?B?dlB2Y3djNHVnNG11VGQ3OE8xamJZTG1Rd3ZiKzVYMWhHdk5YMXp4UVBsakp3?=
 =?utf-8?B?UkVtcjBXb05udCsvZjhSYnp0aVh0bkVWb0QyTTNFdGZEOGhMZGk0aHF6eXFa?=
 =?utf-8?B?OG01ZW1Zay9zL3BSbmMzbGYrTEVEU0hUVWxZYzA0dzhxRnNmUDB4K3JZS1N4?=
 =?utf-8?B?T2tKUjFNRWdxQ1JCMXJneGt6cWM3akYzUS9ZNlZjVVF0VXZVM2hCNHRIclZW?=
 =?utf-8?B?SzN5bVBxQU0xbHZuK1FoNEk0QlM0RjZvM0IrWmJtMWw5VVBSVW1tOWNKeFN3?=
 =?utf-8?B?TGdVcDNjMUh5ZWFrMkNwWnRNVmlXTDgvUUE4SmpzM1R6V2Nabyt0UkxZSnFo?=
 =?utf-8?B?YlFkdE10bThodEdvWWhNL09rSHVSNVRNZHFxWkdwZnAyUThOaFFQcjdkQ3ZB?=
 =?utf-8?B?SnQvRDh4Wi9sNGVkeDI3V0w2Q0JyT3d0OWxEYkNFRlI4WGUzTnlQK2IxNzVV?=
 =?utf-8?B?Sk5KVjRtZUg5SUhGelFheTRDOVpQK255bjE1MXFQVjkvL1NzcXlvYzEzN3JQ?=
 =?utf-8?B?TGw2SEtrM2ZocnRDN0Mvc1J1M1dzTHV6RzNrT3k4b2hLZ283eE5OZm1kQ0d6?=
 =?utf-8?B?VGhBZnJibVdxUktkSksrRmVoZkxFaFV4K0owRU1GcFg1RGpEbzBQY1RjNHE1?=
 =?utf-8?B?c2pPOEUzWFZEL1NwTExKdXZyOW9ONE5xbHBRUnNVUytVWWl1WWk1MENyT3l2?=
 =?utf-8?B?NGdEdW84K2hRcVRzNDkvTWJaZjhmOVZhb0RtT0lHc1VMRkUxcVpBeHMxcXpG?=
 =?utf-8?B?eks1UnhwTnBydjBPbWdsRlljamFYTzdJeDU5NkZrbWVwYTJTNHVzYmZ2emNP?=
 =?utf-8?B?NWNXV1hOMHZCVk8rYndTVE1wVTMxa2d3S25TOE1UMGNScmZyMTFBc2pzRXNF?=
 =?utf-8?B?TDM4YXg2bFdLNlBmdkl0WitQN0puN0w3TVNmQXYvL216ZTNIZTZjS1RaMVEw?=
 =?utf-8?B?YVJZeVZ0cExJeTVWTFZzUHJ3akJRb1BKK01FbG9va21VVVE4TmE2UWVIR3dS?=
 =?utf-8?B?QzdJM01hTEVzWUdjR05OcU52bnhaNFhXWldzMEdpRk1PUnh1cVcrRHd1WjFE?=
 =?utf-8?B?cHM0Vlg0bEpjREhWbnhnYWUreXFrR1hqQStUM2tMZ1N5enFLVkNiVTZxL2Jj?=
 =?utf-8?B?S0pTcDFuQk5Ga1BWVWVUQ1BHbUpnbjg4NHRwbFBQc0xYTUZSM2I4WlE5eDdF?=
 =?utf-8?B?TDlQbGRMdmFzZ0l4ZkRCMzFhMmF1LytZakJ1ejB3ZTk4cDVtTDZDMGgxNzVX?=
 =?utf-8?B?Rk1VNXVqSEVqTXpzcFhXNm5vS09WMlBmRVdETDQ5MFNPMkVSdUluQjJOMTZW?=
 =?utf-8?B?c3htZ1BUc2o2akp3d3hhRkxEZlBpNjdWdnVMTGs1S3ZEUkl4NVdJZmF0RmVF?=
 =?utf-8?B?Y2Zhd0hxOWZHc3VOd0FVbURFaTNsc3pIZFo5TUxtS2VIOHU0REZKdEJvMGF2?=
 =?utf-8?B?VVhmcFYxTkM4T0NmL3hGWnlZYkZvN1I4M2NpVmt5dXQrYmpKa1RBdEkzWExr?=
 =?utf-8?B?Mm1UMGJkVkRuV3pEcTU3VlhpeEhpd2FOTXhSSzlWTUZuWUlmcUZTcW05cVEy?=
 =?utf-8?B?aTlxcHo1amNIbk9kRXhRSEsvc3RlOHY1SXgzZ0Mza0VwSis2cWJrR1Q5WHkw?=
 =?utf-8?B?TzUwdzVIek9DcHJiOXdFbWZoM2J5ZWUrMlUva3djcVVjNmkyWFQ4NU51ZjBS?=
 =?utf-8?B?bXNWYVhPakJhNStBVGk2WlhIYlhUc09kMEpjck5NTytiOGpvRnZ6blpRZXVJ?=
 =?utf-8?B?MW1qZnJuaXdXc3RGVVRGUldwT21SeGRvcERwRWFST2RwaEl4RVVuWmFpV2Ey?=
 =?utf-8?B?ZDN1cXhlMnNUdXJVZnM3RjNSVmFadWJvbjJXbmJSREUvME9lYW9DamQyU2Zx?=
 =?utf-8?B?dnBMU2pvMG9wSVpaUE16dUM1Z3RENkc3QWlFZE4rQUEwOWxkVzdQa1lnaG1X?=
 =?utf-8?B?OU9lS1duNER6MGUrcXNMNGk0N3RTU2JJU3JMdXBvWkV2QWpreUFjNnhWVlR0?=
 =?utf-8?B?T2JYRXVKSkF1K0k5c1JTM0tyYUVoUE1FNE1hWEtXeVgxT3VaZ0djTXNiRnV0?=
 =?utf-8?B?Yk1STWZ3TzVHSHJWRm5TQkUwSDB4eUw4ZVF1RkprcmJJRWx4S2Y4UHZDelln?=
 =?utf-8?B?b3c9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	R7Mi7naTI90njtzvt7Y7HE8egr9LZLezW3+m4D9QFDCTg82VNqFKDgXkz/Vc9DH3LGwur+dY8rLjd9SdlWeDmyPRLa7DGz7vR341hVoALh1+/38rRrOcKzpwChlzSPoZ6gwRE7DY5HHCqme12xLu0p2+foUyPgYBp49ojvRPVWXnpaZ98s+0Wt3hENB44lLlklMJjxjedv+Ox2qypztwPwdHgZ66HxzREQNOrnwODNaTPsfK++zDpxTw5MO7RUAN0C2WggjSRgkZNLjf35FMbRh39Nzd4uToTy/r+zaX0FkCQiATCChEyTo5fjOGgqbcN1uEhqEAxdKVqmYLh9Tf+dNecFD3XSczIers9EgEnc8SPdfNS4XBABuqax1oIFXqj1eEfuLcN14MAirB/qJeRkrTo5Qq/YzYduAhpNyfjRbQiQTj3/bkbKJvWsp0UVTytUUCkRFO6PQFNCBDv82HlOCYkhn6Hphf+QHa8nwJIjZyqhZnngmDPBmydF/9HyXEnqRMEQic5CwSIu8gSU5H+32vHMEf98d8l826jAZfPd7IZK64+IpuJJpmvBPppltL4ZAxvlNVqU7ZUkK6gtxHKUNOIRN7p1Iz0377Y1++npA=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 9f3fc4e6-9403-4d28-c082-08dc2d967554
X-MS-Exchange-CrossTenant-AuthSource: SA2PR10MB4684.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Feb 2024 19:52:21.7984
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: cufAh9D5yDz/ojoU2P8jzCVM/1eeLaln1VjdtoUSihCn1kz8gjw3EJCyImtDujCgZFwmSoT++iW9o3sq9m2ULpJkDhfhk7vH3tUVX0jTRog=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR10MB7389
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.1011,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2024-02-14_12,2024-02-14_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 bulkscore=0 suspectscore=0
 phishscore=0 adultscore=0 spamscore=0 mlxlogscore=999 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2311290000
 definitions=main-2402140155
X-Proofpoint-ORIG-GUID: FHtyLBdd1A86l6-GCG337CDpejLZpFlN
X-Proofpoint-GUID: FHtyLBdd1A86l6-GCG337CDpejLZpFlN

On 2/14/2024 2:39 PM, Eugenio Perez Martin wrote:
> On Wed, Feb 14, 2024 at 6:50 PM Steven Sistare
> <steven.sistare@oracle.com> wrote:
>>
>> On 2/13/2024 11:10 AM, Eugenio Perez Martin wrote:
>>> On Mon, Feb 12, 2024 at 6:16 PM Steve Sistare <steven.sistare@oracle.com> wrote:
>>>>
>>>> Flush to guarantee no workers are running when suspend returns.
>>>>
>>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>>>> ---
>>>>  drivers/vdpa/vdpa_sim/vdpa_sim.c | 13 +++++++++++++
>>>>  1 file changed, 13 insertions(+)
>>>>
>>>> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
>>>> index be2925d0d283..a662b90357c3 100644
>>>> --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
>>>> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
>>>> @@ -74,6 +74,17 @@ static void vdpasim_worker_change_mm_sync(struct vdpasim *vdpasim,
>>>>         kthread_flush_work(work);
>>>>  }
>>>>
>>>> +static void flush_work_fn(struct kthread_work *work) {}
>>>> +
>>>> +static void vdpasim_flush_work(struct vdpasim *vdpasim)
>>>> +{
>>>> +       struct kthread_work work;
>>>> +
>>>> +       kthread_init_work(&work, flush_work_fn);
>>>
>>> If the work is already queued, doesn't it break the linked list
>>> because of the memset in kthread_init_work?
>>
>> work is a local variable.  It completes before vdpasim_flush_work returns,
>> thus is never already queued on entry to vdpasim_flush_work.
>> Am I missing your point?
> 
> No, sorry, I was the one missing that. Thanks for explaining it :)!
> 
> I'm not so used to the kthread queue, but why not calling
> kthread_flush_work on vdpasim->work directly?

vdpasim->work is not the only work posted to vdpasim->worker; see 
vdpasim_worker_change_mm_sync.  Posting a new no-op work guarantees
they are all flushed.

- Steve

>>>> +       kthread_queue_work(vdpasim->worker, &work);
>>>> +       kthread_flush_work(&work);
>>>> +}
>>>> +
>>>>  static struct vdpasim *vdpa_to_sim(struct vdpa_device *vdpa)
>>>>  {
>>>>         return container_of(vdpa, struct vdpasim, vdpa);
>>>> @@ -511,6 +522,8 @@ static int vdpasim_suspend(struct vdpa_device *vdpa)
>>>>         vdpasim->running = false;
>>>>         mutex_unlock(&vdpasim->mutex);
>>>>
>>>> +       vdpasim_flush_work(vdpasim);
>>>
>>> Do we need to protect the case where vdpasim_kick_vq and
>>> vdpasim_suspend are called "at the same time"? Correct userland should
>>> not be doing it but buggy or mailious could be. Just calling
>>> vdpasim_flush_work with the mutex acquired would solve the issue,
>>> doesn't it?
>>
>> Good catch.  I need to serialize access to vdpasim->running plus the worker queue
>> in these two functions.  vdpasim_kick_vq currently takes no locks. In case it is called
>> from non-task contexts, I should define a new spinlock to be acquired in both functions.
>>
>> - Steve
>>
>