Received-SPF: pass (google.com: domain of linux-kernel+bounces-68789-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <5ad9cc63-85db-4cb5-9013-828caf60020d@oracle.com>
Date: Fri, 16 Feb 2024 10:15:45 -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>
 <996cfabf-c612-43c8-9e56-03fb58a1ebec@oracle.com>
 <CAJaqyWdKC6rLBOykSeWc87QtJ757LNXd78R59JRizUyAQCu+ig@mail.gmail.com>
From: Steven Sistare <steven.sistare@oracle.com>
Organization: Oracle Corporation
In-Reply-To: <CAJaqyWdKC6rLBOykSeWc87QtJ757LNXd78R59JRizUyAQCu+ig@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?K2N1SUZ5cko4dEl5NjBqbjZlNW12MzNEckdBOWkvaWdxL1JWY3IyK3JoOVB1?=
 =?utf-8?B?NlF3cXI5SmQxWWRRNU5YeDVrUDhUWHgwdG1DWHBaQjRYKzR4TkloT2VMaTRh?=
 =?utf-8?B?b0RycGdRY08yMk93V3dPMVR1dWRYR05SczRaR2hNeWFtc2JwSXE1QktTSVRQ?=
 =?utf-8?B?ckg3bjhSaWk5cjJuWWVmdHc5WU1KbE5NZmFJenh0dGIvb1ZOMXFFcjd5dk9u?=
 =?utf-8?B?aU5PV05PbEFZM2xrWUxuRkVMb0RCRmlOcTFXY0c0dDVZY21TN0Nhb1c1TEx1?=
 =?utf-8?B?R2oyMWVzVjYvZWZTNnZhVC91WWlQa0pPRXg5V2t5UzdhZmRiakdiRjQ0djJs?=
 =?utf-8?B?SUNkYUlNYnlDa1ZLalNtTk9yTldVVzU2L3VLbEdXR3I0SFo4eHNTeVhCajJG?=
 =?utf-8?B?Z21DK3h2TTU5MXF1VEtlYW5XcHllTzEyQzQ0dGhMRGY1ZlRRWE5FUGp1YmNw?=
 =?utf-8?B?RnhRUVVYV3czc3phOFU4ZXVMK0JpdUVkWE9aOWUyTXFkRzZYUGo4alBicWNT?=
 =?utf-8?B?MVdxeWtBbnhMS3RKdW52S0h0RDYzVS8rNG1PZHhLa24zS2dYcVd6VyszYmZQ?=
 =?utf-8?B?WEdaVVNuTmNHR3BUQSt2YjE4ZHJqaWwyN3h5UUVSWmR1SDRHTzl1U25QdEFC?=
 =?utf-8?B?ZW12TDJ5cjFpNnlVMDkwaUFyWXY1ZmVjdkF1TytpTmpIN3c0bWFWcTN2SEI0?=
 =?utf-8?B?UVUxeGJyMEV5V3dCRmNEVjM3T05hZGlKM0xyQWRVTDVNZmdVemM5ZzJOZ0c3?=
 =?utf-8?B?SHJFK3ZZbW9qQmZLMDZkNWV0R3BENGpHNWUwdzVBSHVOVVpKMFJwODcrMXRp?=
 =?utf-8?B?cnVQNjJKak8yOFdDbWNIY3ROZ1loSU5UbUlZUDJoaHQ2aXVURUcyVlVZQ3I4?=
 =?utf-8?B?VnNpd2ZvY0dPUm8yRFdYcWt6ZjlPZWRFUHNyRHA5NUZRNGY5UEV0cjdMRlNO?=
 =?utf-8?B?WG53cythNEExYXB2N3ExTU5nU3VRWG4vWFF0VkxsbVBhdjlsM095YTZ2VHdX?=
 =?utf-8?B?dkgwMEdrZ0ovZ0xGWldoTzBURnBsbEViWlJ2SDJhNnpxdFJBclhhZy83U2ZK?=
 =?utf-8?B?VHIyNlZQT3FFMVBDSEdMYUpiOTdxYzNBU1hKVWNMNFBFeGZIZkZQY3poeWYy?=
 =?utf-8?B?bk9uYk1RYzh6UVYwVEMyY3AwY3hJa0xWaXkrdVdsd1BWRjlvK0liTHF2ZENi?=
 =?utf-8?B?eStucDJ0UU5xcGUyUlZxeFZVNkJHSWdGOWNINHBhVG0xb3FRRVR5SEJUZGdm?=
 =?utf-8?B?MnpwaFV6ZndSd0lmdk55VXREakhRM1NzeFVhenVXQnoxZitsaUkzdHV1VkRP?=
 =?utf-8?B?dmtoLzY2Y3NhdWNYYTU5ZjNqRFc5YzVmSWFnRjdkUURQV1VyeW4zRXFXUnN1?=
 =?utf-8?B?YUFXOEVNalo5bEdvMG0yeEVsaEc2Y2hvaDZxK0lXeGl4dURybmJhNE5FZW5N?=
 =?utf-8?B?SU1QWDZ4QThhU3hCNU5vSnA2eXYrMU9vSXI1YWtkQlVWK1JWOXhHQ3dMWUg2?=
 =?utf-8?B?enFOSEFTakRoS011dHNpK2FDZXI5VWE1WlZPOS84WEVOWDR5dlBycnlEM1Zj?=
 =?utf-8?B?U0JWTCttNXNFc3VMS3R1a0tWUXdXdlBkVHZja0ZXRFVLeUFNRVRNZ2JoUzlM?=
 =?utf-8?B?dGJsenZJemhiMEUyYmIvTHZqcWs0WkdFU1ZEZ2xkYTJzNFhXVHZrckZtanV2?=
 =?utf-8?B?V2FzaU5zazBGNlVUVXVCS25wWEVMeVo1c2d5WHBzZlpSaWJoL0pmTTF0bjV3?=
 =?utf-8?B?WHlsWUhSam9NY2xEazRXdmlOL0MvbXhWL2xYUjhUUGtKZW5iN09acEZuQTcv?=
 =?utf-8?B?UmlmKzFVbDJreEdITGVNWnE4VHR5eThTU2ovZkdWbFcrNWRVdUNnaEtWWE9U?=
 =?utf-8?B?TXlaZW9zTGhEcjZNblpzSDFKTjdIZGU4Q3FHM1N0L2Q4d2U0MnFVTVZRUEsz?=
 =?utf-8?B?cVdQcHVJcTFhNTNaWWF4Z0QvNm5ucVp6RmF2SlEvVDV4SHZmSFJVKzE5TDZC?=
 =?utf-8?B?bzVrZGhBY05zK1d6NDgwNWg5QmMyc1U0OTFZaGVmU3hlaG1BUk92NlNPMlJT?=
 =?utf-8?B?RGxEN2w0cFhCcDl3Wkx3M1BpVVJmMGd2LzlWMG5ZQXZ0VlRSRExVWEFJNTZv?=
 =?utf-8?B?UWNva2lGTk4zem93ZlZtcEtjL0k1ajlWUG9neXh6SUZFN0w5REVCSi84R0Vt?=
 =?utf-8?B?Vmc9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	VuRWzYDHCOi49Fo/yeZDJdB2MT8g3TBMo+19Qm0C8dSPLAQLSiq1kiToNWBNIU/lRMT3LnUR8Ec6pOHpSx9otwyoSeMs+sG0EkgZfvlc1/bu4O29VTy9i/JIQfdWU+n1HZebNg5MzP5ycbm/ezeL3ehF291fphXG0NtzUtp1k0Cpy+OQDMrq7579/6JCbjJOXGAjzWQDy9lw/Nv18IBG/8Q6EqNv1f1zL3leT/1umI6qQF5LJNMo1TXPtzKnjNInjqRtN3BXrbMoILUpH+P5mkBH4jEpaouSQHWcC2lith0Hqk+tZFC5Zx/q0nWHoyTopJxD2xwdTTdyb7u7TlEQJ+R+0qykH8BSzPY7O/HAyrCHO5Aqfx7zI7eq3J4vomHA2K9y/Xv1M+ubOJ1zvvP7dgqw65tH8K4Q5kmYuH3eibPPkotaynRKVuaMl5x7RdrXonoOwKniF09w48M2fSdrq2Rx1AjrN6mAlzDkoZShtn6nHG+98Zt7HKRvT31awSRRSpHCtuCqt2qyGjD93oI1L3r3hPa7SpKLL24cvTQ/MNC0LRwTLita0riw9pMaEF6gGjWxBtoyITH+0slKnxbjNoULYI6F3uyUWxxgJIz0pBA=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 9195c6d1-d98a-4589-97a1-08dc2f02295f
X-MS-Exchange-CrossTenant-AuthSource: SA2PR10MB4684.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Feb 2024 15:15:51.1571
 (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: UaQUMd4OwzoiyLw8+y0XtBMMAX0aTgnwR4sLDAfdWsN/Ss3hQBH7r2w2KTYUiYXGvHhmDVYRtMGswoGCY20arTTYOfuDU1UMtOl11XXecFY=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ2PR10MB7618
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-16_15,2024-02-16_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 bulkscore=0
 mlxlogscore=999 malwarescore=0 mlxscore=0 spamscore=0 suspectscore=0
 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2311290000 definitions=main-2402160122
X-Proofpoint-GUID: 0UBq3bb51ln--4taJSY0Vx4eCCoLILO3
X-Proofpoint-ORIG-GUID: 0UBq3bb51ln--4taJSY0Vx4eCCoLILO3

On 2/15/2024 10:44 AM, Eugenio Perez Martin wrote:
> On Wed, Feb 14, 2024 at 8:52 PM Steven Sistare
> <steven.sistare@oracle.com> wrote:
>>
>> 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.
> 
> But it is ok to have concurrent mm updates, isn't it? Moreover, they
> can be enqueued immediately after the kthread_flush_work already, as
> there is no lock protecting it.

Agreed on both, thanks.  I will simplify and only flush vdpasim->work.

- 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
>>>>
>>>
>>
>