On 9.09.2024 21:40, Peter Xu wrote:
On Fri, Aug 30, 2024 at 10:02:40AM -0300, Fabiano Rosas wrote:
@@ -397,20 +404,16 @@ bool multifd_send(MultiFDSendData **send_data)
p = &multifd_send_state->params[i];
/*
- * Lockless read to p->pending_job is safe, because only multifd
- * sender thread can clear it.
+ * Lockless RMW on p->pending_job_preparing is safe, because only
multifd
+ * sender thread can clear it after it had seen p->pending_job being
set.
+ *
+ * Pairs with qatomic_store_release() in multifd_send_thread().
*/
- if (qatomic_read(&p->pending_job) == false) {
+ if (qatomic_cmpxchg(&p->pending_job_preparing, false, true) == false) {
What's the motivation for this change? It would be better to have it in
a separate patch with a proper justification.
The original RFC patch set used dedicated device state multifd channels.
Peter and other people wanted this functionality removed, however this caused
a performance (downtime) regression.
One of the things that seemed to help mitigate this regression was making
the multifd channel selection more fair via this change.
But I can split out it to a separate commit in the next patch set version and
then see what performance improvement it currently brings.
Yes, better to have it separate if anything for documentation of the
rationale.
And when drafting that patch, please add a comment explaining the field.
Currently it's missing:
/*
* The sender thread has work to do if either of below boolean is set.
*
* @pending_job: a job is pending
* @pending_sync: a sync request is pending
*
* For both of these fields, they're only set by the requesters, and
* cleared by the multifd sender threads.
*/
bool pending_job;
bool pending_job_preparing;
bool pending_sync;
Will do if these variables end staying in the patch (instead of being
replaced by the common send mutex, for example).
Thanks,
Maciej
