Re: [PATCH] KVM: x86: use assign_bit() where applicable

[Alison Schofield]

On Mon, Sep 28, 2026 at 05:58:57PM -0700, Sean Christopherson wrote:
> On Sun, Sep 20, 2026, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@xxxxxxx>
> > 
> > Convert open-coded if/else with set_bit/clear_bit and their
> > non-atomic __set_bit/__clear_bit variants to the assign_bit/__assign_bit
> > API.
> 
> ...
> 
> > Signed-off-by: Peng Fan <peng.fan@xxxxxxx>
> > ---
> >  arch/x86/kvm/hyperv.c  | 12 ++++--------
> >  arch/x86/kvm/svm/pmu.c |  6 ++----
> >  arch/x86/kvm/x86.c     | 11 +++--------
> >  3 files changed, 9 insertions(+), 20 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> > index 8d2669d8ef34..c131d9a3c550 100644
> > --- a/arch/x86/kvm/hyperv.c
> > +++ b/arch/x86/kvm/hyperv.c
> > @@ -114,17 +114,13 @@ static void synic_update_vector(struct kvm_vcpu_hv_synic *synic,
> >  	if (vector < HV_SYNIC_FIRST_VALID_VECTOR)
> >  		return;
> >  
> > -	if (synic_has_vector_connected(synic, vector))
> > -		__set_bit(vector, synic->vec_bitmap);
> > -	else
> > -		__clear_bit(vector, synic->vec_bitmap);
> > +	__assign_bit(vector, synic->vec_bitmap,
> > +		     synic_has_vector_connected(synic, vector));
> >  
> >  	auto_eoi_old = !bitmap_empty(synic->auto_eoi_bitmap, 256);
> >  
> > -	if (synic_has_vector_auto_eoi(synic, vector))
> > -		__set_bit(vector, synic->auto_eoi_bitmap);
> > -	else
> > -		__clear_bit(vector, synic->auto_eoi_bitmap);
> > +	__assign_bit(vector, synic->auto_eoi_bitmap,
> > +		     synic_has_vector_auto_eoi(synic, vector));
> 
> Am I the only one that finds the assign_bit() code signficantly harder to follow?
> Maybe it's just that I haven't seen assign_bit() much, but I've come back to this
> patch several times, and I've had the same reaction every time.  IMO, this is a
> solution looking for a problem.


A place for me to pile on, hopefully constructively. :)

I looked at the DAX patch doing same initially and set it aside. After a few
review tags came in, I took a closer look and decided to NAK it.

A few things contributed to that decision.

These patches were sent individually, all with "where applicable" in the subject,
but without explaining why the conversion was appropriate in each case. That leaves
reviewers to establish the justification for the change, rather than evaluate the
justification provided by the author.

I would have preferred to see these as a series, so reviewers could see the scope of
the proposed conversions and discuss the approach as a whole.

Looking through the history of assign_bit(), I found that it was introduced for a
specific use case, then later moved into bitops.h when someone else had a need for it.
I didn't find any indication that the existing if/else pattern was considered
problematic or that there was an intent to replace it more broadly.

In the DAX case, the conversion /drivers/dax/super.c` didn't appear to simplify the code
or make the intent any clearer.

I'm not opposed to using `assign_bit()` where it improves the code, but I don't think the
existence of a helper is, by itself, sufficient justification for converting existing code.
I'd rather see these conversions motivated by a concrete improvement than by the
opportunity to replace an open-coded pattern.

-- Alison