[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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).

--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.