From: Boqun Feng <boqun.feng@xxxxxxxxx> Sent: Monday, June 24, 2024 10:55 AM
>
> Hi Michael,
>
> On Mon, Jun 03, 2024 at 10:09:39PM -0700, mhkelley58@xxxxxxxxx wrote:
> [...]
> > diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
> > index bf35bb40c55e..571b2955b38e 100644
> > --- a/drivers/hv/hyperv_vmbus.h
> > +++ b/drivers/hv/hyperv_vmbus.h
> > @@ -264,6 +264,14 @@ struct vmbus_connection {
> > struct irq_domain *vmbus_irq_domain;
> > struct irq_chip vmbus_irq_chip;
> >
> > + /*
> > + * VM-wide counts of MODIFYCHANNEL messages sent and completed.
> > + * Used when taking a CPU offline to make sure the relevant
> > + * MODIFYCHANNEL messages have been completed.
> > + */
> > + u64 modchan_sent;
> > + u64 modchan_completed;
> > +
>
> Looks to me, we can just use atomic64_t here: modifying channels is far
> from hotpath, so the cost of atomic increment is not a big issue, and we
> avoid possible data races now and in the future.
>
> Thoughts?
At one point in the development, I did have these as atomic64_t.
And I agree that the usage is pretty infrequent, so the atomic costs
aren't an issue. But both fields are already safe. modchan_sent
is covered by vmbus_connection.set_affinity_lock as held in
vmbus_irq_set_affinity(). And modchan_completed is OK because
it is only ever incremented by code running on VMBUS_CONNECT_CPU,
per the comment in vmbus_onmodifychannel_response().
Converting to atomic64_t wouldn't hurt anything, and arguably
would provide additional robustness, but it just didn't seem
necessary.
Michael
