Re: [XEN PATCH][for-4.19 v4 1/8] xen/include: add macro ISOLATE_LOW_BIT

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 17 Nov 2023, Nicola Vetrini wrote:
> Hi Jan,
> 
> > > > > > While I've committed this patch (hoping that I got the necessary
> > > > > > context
> > > > > > adjustment right for the
> > > > > > automation/eclair_analysis/ECLAIR/deviations.ecl
> > > > > > change), I'd like to come back to this before going further with
> > > > > > users
> > > > > > of
> > > > > > the new macro: I still think we ought to try to get to the single
> > > > > > evaluation wherever possible. The macro would then be used only in
> > > > > > cases
> > > > > > where the alternative construct (perhaps an isolate_lsb() macro,
> > > > > > living
> > > > > > perhaps in xen/bitops.h) cannot be used. ISOLATE_LSB() would then
> > > > > > want
> > > > > > to
> > > > > > gain a comment directing people to the "better" sibling. Thoughts?
> > > > > 
> > > > > Having the users in place would help me estimate the remaining work
> > > > > that
> > > > > needs to be done on this rule and see if my local counts match up with
> > > > > the counts in staging.
> > > > 
> > > > By "having the users in place", you mean you want other patches in this
> > > > and the dependent series to be committed as-is (except for the name
> > > > change)? That's what I'd like to avoid, as it would mean touching all
> > > > those use sites again where the proposed isolate_lsb() could be used
> > > > instead. I'd rather see all use sites be put into their final shape
> > > > right away.
> > > 
> > > This request is coming a bit late and also after all the patches have
> > > been reviewed already. I for one am not looking forward to review them
> > > again.
> > > 
> > > That said, if you could be more specified maybe it could become
> > > actionable:
> > > 
> > > - do you have a pseudo code implementation of the "better" macro you
> > >   would like to propose?
> > 
> > May I remind you that I made this request (including a draft implementation)
> > before already, and Nicola then merely found that the evaluate-once form
> > simply cannot be used everywhere? Anybody could have thought of the option
> > of "splitting" the macro. After all I hope that there is no disagreement on
> > macro arguments better being evaluated just once, whenever possible.
> > 
> > > - do you have an list of call sites you would like to be changed to use
> > >   the "better" macro?
> > 
> > No, I don't have a list. But the pattern is pretty clear: The "better" form
> > ought to be used wherever it actually can be used.
> > 
> > > Also, you might remember past discussions about time spent making
> > > changes yourself vs. others doing the same. This is one of those cases
> > > that it would be faster for you to make the change and send a patch than
> > > explaining someone else how to do it, then review the result (and
> > > review it again as it probably won't be exactly as you asked the first
> > > time.)
> > > 
> > > If you don't want the call sites to be changes twice, may I suggest you
> > > provide a patch on top of Nicola's series, I review and ack your patch,
> > > and Nicola or I rebase & resend the series so that the call sites are
> > > only changes once as you would like? I think that's going to be way
> > > faster.
> > 
> > I'll see if I can find time to do so. I don't normally work on top of
> > other people's uncommitted patches, though ... So I may also choose to go
> > a slightly different route. (You realize though that all still pending
> > patches using the new macro need touching again anyway, don't you?)
> > 
> > Jan
> 
> Then perhaps it's best if I give it a try at doing the single evaluation
> macro, so that I can make a series modifying the call sites only once on top
> of that and send everything in one go. Before doing that, though, I'll make a
> thread where various aspects that are not so clear yet can be discussed, so
> that we can devise a robust solution (also to dig this out of this deep
> thread).

In the meantime I committed patches from #5 onward as they don't depend
on ISOLATE_LSB