[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
On 17.11.2023 01:43, Stefano Stabellini wrote: > On Thu, 16 Nov 2023, Jan Beulich wrote: >> On 16.11.2023 11:02, Nicola Vetrini wrote: >>> On 2023-11-16 09:26, Jan Beulich wrote: >>>> On 31.10.2023 11:20, Jan Beulich wrote: >>>>> On 31.10.2023 11:03, Nicola Vetrini wrote: >>>>>> On 2023-10-31 09:28, Nicola Vetrini wrote: >>>>>>> On 2023-10-31 08:43, Jan Beulich wrote: >>>>>>>> On 30.10.2023 23:44, Stefano Stabellini wrote: >>>>>>>>> On Mon, 30 Oct 2023, Jan Beulich wrote: >>>>>>>>>> On 27.10.2023 15:34, Nicola Vetrini wrote: >>>>>>>>>>> --- a/xen/include/xen/macros.h >>>>>>>>>>> +++ b/xen/include/xen/macros.h >>>>>>>>>>> @@ -8,8 +8,14 @@ >>>>>>>>>>> #define DIV_ROUND(n, d) (((n) + (d) / 2) / (d)) >>>>>>>>>>> #define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d)) >>>>>>>>>>> >>>>>>>>>>> -#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m))) >>>>>>>>>>> -#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m)) >>>>>>>>>>> +/* >>>>>>>>>>> + * Given an unsigned integer argument, expands to a mask where >>>>>>>>>>> just the least >>>>>>>>>>> + * significant nonzero bit of the argument is set, or 0 if no >>>>>>>>>>> bits >>>>>>>>>>> are set. >>>>>>>>>>> + */ >>>>>>>>>>> +#define ISOLATE_LOW_BIT(x) ((x) & -(x)) >>>>>>>>>> >>>>>>>>>> Not even considering future Misra changes (which aiui may require >>>>>>>>>> that >>>>>>>>>> anyway), this generalization of the macro imo demands that its >>>>>>>>>> argument >>>>>>>>>> now be evaluated only once. >>>>>>>>> >>>>>>>>> Fur sure that would be an improvement, but I don't see a trivial >>>>>>>>> way >>>>>>>>> to >>>>>>>>> do it and this issue is also present today before the patch. >>>>>>>> >>>>>>>> This was an issue here for MASK_EXTR() and MASK_INSR(), yes, but >>>>>>>> the >>>>>>>> new >>>>>>>> macro has wider use, and there was no issue elsewhere so far. >>>>>>>> >>>>>>>>> I think it >>>>>>>>> would be better to avoid scope-creeping this patch as we are >>>>>>>>> already >>>>>>>>> at >>>>>>>>> v4 for something that was expected to be a trivial mechanical >>>>>>>>> change. >>>>>>>>> I >>>>>>>>> would rather review the fix as a separate patch, maybe sent by you >>>>>>>>> as >>>>>>>>> you probably have a specific implementation in mind? >>>>>>>> >>>>>>>> #define ISOLATE_LOW_BIT(x) ({ \ >>>>>>>> typeof(x) x_ = (x); \ >>>>>>>> x_ & -x_; \ >>>>>>>> }) >>>>>>>> >>>>>>>> Hard to see the scope creep here. What I would consider scope creep >>>>>>>> I >>>>>>>> specifically didn't even ask for: I'd like this macro to be >>>>>>>> overridable >>>>>>>> by an arch. Specifically (see my earlier naming hint) I'd like to >>>>>>>> use >>>>>>>> x86's BMI insn BLSI in the context of "x86: allow Kconfig control >>>>>>>> over >>>>>>>> psABI level", when ABI v2 or higher is in use. >>>>>>> >>>>>>> I appreciate you suggesting an implementation; I'll send a v5 >>>>>>> incorporating it. >>>>>> >>>>>> There's an issue with this approach, though: since the macro is used >>>>>> indirectly >>>>>> in expressions that are e.g. case labels or array sizes, the build >>>>>> fails >>>>>> (see [1] for instance). >>>>>> Perhaps it's best to leave it as is? >>>>> >>>>> Hmm. I'm afraid it's not an option to "leave as is", not the least >>>>> because >>>>> - as said - I'm under the impression that another Misra rule requires >>>>> macro arguments to be evaluated exactly once. Best I can think of >>>>> right >>>>> away is to have a macro for limited use (to address such build issues) >>>>> plus an inline function (for general use). But yes, maybe that then >>>>> indeed >>>>> needs to be a 2nd step. >>>> >>>> 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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |