Received-SPF: pass (google.com: domain of linux-kernel+bounces-29419-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <e87ca0a4-9912-441a-b31f-d11c41ad6f5d@oracle.com>
Date: Wed, 17 Jan 2024 15:31:06 -0500
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC V1 10/13] 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>,
        Xuan Zhuo <xuanzhuo@linux.alibaba.com>,
        Dragos Tatulea <dtatulea@nvidia.com>, Eli Cohen <elic@nvidia.com>,
        Xie Yongji <xieyongji@bytedance.com>
References: <1704919215-91319-1-git-send-email-steven.sistare@oracle.com>
 <1704919215-91319-11-git-send-email-steven.sistare@oracle.com>
 <CAJaqyWdTBexyY-ra=UkUfUU75YS2Zncm5-9iHv-0Rqa3_mD=9A@mail.gmail.com>
From: Steven Sistare <steven.sistare@oracle.com>
Organization: Oracle Corporation
In-Reply-To: <CAJaqyWdTBexyY-ra=UkUfUU75YS2Zncm5-9iHv-0Rqa3_mD=9A@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?YTVrQ3RWRkNja0crZkFYRnlpQ01lMWllcUlHQWV1VnJEQkV3cy9QZjdsQ3Z2?=
 =?utf-8?B?cFhhTVpRVmQxdDlpVUVLTVY3cTVWamR4cUNQMmd1dlYwNDNVTjlDMys0eG04?=
 =?utf-8?B?V3BaZzdZNGxoajdwTkJTN0VtTmxsc08zSk9UVVhLdnRBdDd5dUQ1NWp3Uk9D?=
 =?utf-8?B?QVhaNEMrVEtnL0ZuQ3VGVXB3RXVmMjd5eU43WmozaUhuWGFKcUxYaW5tVmRG?=
 =?utf-8?B?UVhGNmtDSTdpL2ZNeEdqaGNsK2ZSM2JRTlEwT0M3WkZQMDRmSkhqRGZ5cHY3?=
 =?utf-8?B?OFZibE5jWUQ3MHRqa21MMDhrV05sSFpIdmFHK2FaV3pnMHNpb2tKQVZMUnNP?=
 =?utf-8?B?cVg1dFdTMGxrdlVaMjVQdE9sOXkxWXJMMzhrdGtHLzdlTGZHT1hjWUtRWDRT?=
 =?utf-8?B?QUhLbVRBM3Q1bk9Fam5Wa29RVC9GWFJtcXZoLzl1SGtmaWJQQi83cUVsQ3BC?=
 =?utf-8?B?QndXbTNQQnpqWXNiOEhQVzd6b0Y5eVFtNlB2NTRNV0Z1dng3TndmQndPOXBn?=
 =?utf-8?B?OG8xOStMY3VnWUg3RUxHTjZZKzlFektIYkJEZlF3Tml2eWdQbVliOFBtMEZX?=
 =?utf-8?B?QXBGOFZkTjcxZzZUSzhVMCsxNGVLeC9zL0piRXEzai9RN2NKZGVrRWJEQ3F4?=
 =?utf-8?B?YXFCVmxwU1NyWjVsYVdhNElUNnY5Zit6NUN4WENLOTIzbWsxK0Ruck50NXow?=
 =?utf-8?B?b0l2YitUYk8yaWVuSzhPMmllcW1PUGNvSXRabFFyNnUrYWlPZ2NGeDJpd2ZG?=
 =?utf-8?B?TjVpQktaam4xOXpvZ1NtQ2FZeExDdzhFYlVHS0x3WHo4emZaYUM5QWMvM3dW?=
 =?utf-8?B?RXlEQ1VHcDhmZjd1TS9yb210dWd1enh5b3UvV3BYRFd4NlFoRmMvMXdHeUJ0?=
 =?utf-8?B?UHNYTjNrSWdLRCtsOTI0bXlmQ2ZNcVNRck5xNUw5Tkw3em1Eb1FKTjNlWDRq?=
 =?utf-8?B?T3BnZWlTclRUTFBLR2VqbTZ4TDJYVndTZndNVnY2bmZWNG5RMjVTckYzUE4w?=
 =?utf-8?B?bWxpckdtdjAySlkveTkvNU8xR3l4U3ZPYXNUMEIzYVpBTmpJV2tQMTlvcHRX?=
 =?utf-8?B?VjJUcXJ3RmlQODZVTjYxMjNVbEMxVFhkRk5mY1dBQitHcTUyMnI3NGN1T0I5?=
 =?utf-8?B?ZE51M2lVOXlqWXlqSG0za0ovcHBWK1F2TndkUDBKSXI2SnlFd3l1U1BFcWtB?=
 =?utf-8?B?RUhERXlHYkhOdjM5elg1WTRub3c2cmZPRG56a3NITmJBY2VBSzBLanY2Yy9I?=
 =?utf-8?B?TzFPMkx2d05WNU0zUm9nU2JTS1kycjRMdU5IbVJYak9YaFRTN2F1STkrTzll?=
 =?utf-8?B?N1AzMFBGQmdnZUFKSnRrZzNmeWRZUXNrMlJWR2lIMGlqZ0hDWStoU2JGc2kx?=
 =?utf-8?B?ckkzM0t5T2o4T1FzYWkwcWRUbDBzSDRxb3g4cVdTdDVNaGE1QVR0cmRwL2lr?=
 =?utf-8?B?alNsSnYzbjZzdU5SVDdxeXlKbmwwZmhJaVpjOThYUytIS1crUVVPYitSdExs?=
 =?utf-8?B?YTlhdDBoR2J5dG9hVnlmejZDK1VGODB3d0ZLa3BJUTQ2cWE3MUFkdmVpaWI0?=
 =?utf-8?B?bnNRZnNuRzA3UDdyendDL2tGRk4vdGpENk5RYXN2V3Y5ZVduUkhCMnVVM2tB?=
 =?utf-8?B?a01YcWpmOC9pRjZVbENKM2tQVVV6WWttVHZUWkZ5MWd5MERIcFh2QmdiVWJh?=
 =?utf-8?B?b2Z3V3hJWkIwRlFNZHpMOGw4MFlwSUozV2Q4N1JGWU5ZNloxVFd0OHhHVHpo?=
 =?utf-8?B?ZTl4VWtGelNMK0JkVGQvdGN3cVpoMmJWNG9zY0FMREpzU3duTzlpV3RadExX?=
 =?utf-8?B?SXpidVVIbHN4QTZJbDZ5YjJGV0p1WnZ2R05uK2hkV1NnL204Q1Z0cFlGZW9Z?=
 =?utf-8?B?d244SEF5K0lUeFJOdjVHbWQ0N2ExRUNNNC9hR1M0MkwvVGQyeFNmb1NXTVVt?=
 =?utf-8?B?eXI5MjhNMitKMlExdXc5dTMwU1UvOHdGS2d2NXVHS1Y1ZTNDUXR4eDhvZDFn?=
 =?utf-8?B?bTByaVZPaWVEK1FEbDcycEFQMGxVZWVzaHl0TW1GamEzU3VtMnVtcE9BcTIv?=
 =?utf-8?B?SFhaa3lXRjhVWWphUGJlei81U1dYNFh2N01ITzRBS3g4Y21LK1phUnU2b2ZF?=
 =?utf-8?B?Zkt0TUdQd2oxeTBtRFNjMVloUW9hOXVKUXVxK29mTFpRT2o2QWxPREtwL0cr?=
 =?utf-8?B?b3c9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	7rfjc5QJQ44POEAlxueMXWCmzUgDxZQkeY/Wd8j3xQE/6xs9FhiyLFnyNSl7fA0oSjYPGv5iAzbqedT/dByXv+VyaWVZ+x53YKwWMevNKwOzBqIZrCv54Nq0vW2OMelyc6rL5pjDF3tnnF5sR3fvGsKLvq4Mo4nRzKhcrquinjz8ytFYONc7qCOwbyQ/EdsMnMo+TVzSmnj0kraURliOt4WpgA4BOBpWmHLAKgAxdg91m3yHEMeyxNVMA9Ei+bii2OSYCeuCNCJtTfaFPk7SnsFAM5HC97zyL3CHItH2cBJt3UZ887g4F9h2JsqUNPP/SJAal4S4aphfqf+vUT7kWI5W6CEs6jf/OCTyhluPMwtiKDi1+1aoUvBVbu4YiLfig262uFJvDIrUddBLb99Iq4loFLE7r08dv+JD54d5AHLEw5tk6/o6sd6mUEe7mbNQcP0fg+nGnTRRH2lTdvE4DwLygU2wv4p/9cccMnK3MWsJbwCf81NSNJtAM5R+GoWC50hPYIYchNzReOKVymHryzubCHBNhBTxYUbewR+r67w/KAMN7Dccj1e5pC0pu7VstesycC9CQuhHEnmVLhHzPPUDwoMh+pHYPSVjw1FAbOY=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: a3217d26-2dce-4ce5-bdd0-08dc179b3cb2
X-MS-Exchange-CrossTenant-AuthSource: SA2PR10MB4684.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Jan 2024 20:31:08.7037
 (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: PCYD2fv+du06MSmBus5UpLccu6N+hKruyU20M7ZHRmbaIKq7iInG86x8XUm17CauG1wubBnHhOLWKkwx1wNGT1Ykkxo+0fJN+YLbxHJK3VM=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR10MB5942
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.997,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2024-01-17_12,2024-01-17_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 bulkscore=0 suspectscore=0
 phishscore=0 spamscore=0 adultscore=0 mlxlogscore=999 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2311290000
 definitions=main-2401170150
X-Proofpoint-GUID: -mVaedBJlQbPy1Zqhk4eqwwmoMY5V4LP
X-Proofpoint-ORIG-GUID: -mVaedBJlQbPy1Zqhk4eqwwmoMY5V4LP

On 1/16/2024 1:57 PM, Eugenio Perez Martin wrote:
> On Wed, Jan 10, 2024 at 9:40 PM Steve Sistare <steven.sistare@oracle.com> wrote:
>>
>> To pass ownership of a live vdpa device to a new process, the user
>> suspends the device, calls VHOST_NEW_OWNER to change the mm, and calls
>> VHOST_IOTLB_REMAP to change the user virtual addresses to match the new
>> mm.  Flush workers in suspend to guarantee that no worker sees the new
>> mm and old VA in between.
> 
> The worker should already be stopped by the end of the suspend ioctl,
> so maybe we can consider this a fix?

Do you mean: the current behavior is a bug, independently of my new use case,
so I should submit this patch as a separate bug fix?  If yes, then will do.

>> 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 6304cb0b4770..8734834983cb 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);
>> +       kthread_queue_work(vdpasim->worker, &work);
>> +       kthread_flush_work(&work);
> 
> Wouldn't it be better to cancel the work with kthread_cancel_work_sync here?

I believe that does not guarantee that currently executing work completes:

  static bool __kthread_cancel_work_sync()
    if (worker->current_work != work)
        goto out_fast;

- Steve

>> +}
>> +
>>  static struct vdpasim *vdpa_to_sim(struct vdpa_device *vdpa)
>>  {
>>         return container_of(vdpa, struct vdpasim, vdpa);
>> @@ -512,6 +523,8 @@ static int vdpasim_suspend(struct vdpa_device *vdpa)
>>         vdpasim->running = false;
>>         mutex_unlock(&vdpasim->mutex);
>>
>> +       vdpasim_flush_work(vdpasim);
>> +
>>         return 0;
>>  }
>>
>> --
>> 2.39.3
>>
>