- add parentheses where they were missing (MISRA)
- make sure to evaluate also v exactly once (MISRA)
- remove excess parentheses
- rename local variables to not have leading underscores
- apply Xen coding style
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
I wonder whether "if ( n_ )" is really helpful: It's extra code in all
callers passing a non-constant size, just to cover a pretty rare case
which memset() is required to deal with correctly anyway, and which
__memzero() also handles quite fine from all I can tell.
--- a/xen/arch/arm/include/asm/string.h
+++ b/xen/arch/arm/include/asm/string.h
@@ -28,17 +28,19 @@
void __memzero(void *ptr, size_t n);
-#define memset(p, v, n) \
- ({ \
- void *__p = (p); size_t __n = n; \
- if ((__n) != 0) { \
- if (__builtin_constant_p((v)) && (v) == 0) \
- __memzero((__p),(__n)); \
- else \
- memset((__p),(v),(__n)); \
- } \
- (__p); \
- })
+#define memset(p, v, n) \
+ ({ \
+ void *p_ = (p); size_t n_ = (n); \
+ typeof(v) v_ = (v); \
+ if ( n_ ) \
+ { \
+ if ( __builtin_constant_p(v) && !v_ ) \
+ __memzero(p_, n_); \
+ else \
+ memset(p_, v_, n_); \
+ } \
+ p_; \
+ })
#endifHi Jan,
> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jan
> Beulich
> Sent: 2022年8月19日 15:50
> To: xen-devel@lists.xenproject.org
> Cc: Julien Grall <julien@xen.org>; Stefano Stabellini
> <sstabellini@kernel.org>; Volodymyr Babchuk <volodymyr_babchuk@epam.com>;
> Bertrand Marquis <Bertrand.Marquis@arm.com>
> Subject: [PATCH 2/2] Arm32: tidy the memset() macro
>
> - add parentheses where they were missing (MISRA)
> - make sure to evaluate also v exactly once (MISRA)
> - remove excess parentheses
> - rename local variables to not have leading underscores
> - apply Xen coding style
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> I wonder whether "if ( n_ )" is really helpful: It's extra code in all
> callers passing a non-constant size, just to cover a pretty rare case
> which memset() is required to deal with correctly anyway, and which
What rare case we need to use n_ that can make memset happy? As your
comment, I read the code again, but it seems to work fine without n_.
Cheers,
Wei Chen
> __memzero() also handles quite fine from all I can tell.
>
> --- a/xen/arch/arm/include/asm/string.h
> +++ b/xen/arch/arm/include/asm/string.h
> @@ -28,17 +28,19 @@
>
> void __memzero(void *ptr, size_t n);
>
> -#define memset(p, v, n) \
> - ({ \
> - void *__p = (p); size_t __n = n; \
> - if ((__n) != 0) { \
> - if (__builtin_constant_p((v)) && (v) == 0) \
> - __memzero((__p),(__n)); \
> - else \
> - memset((__p),(v),(__n)); \
> - } \
> - (__p); \
> - })
> +#define memset(p, v, n) \
> + ({ \
> + void *p_ = (p); size_t n_ = (n); \
> + typeof(v) v_ = (v); \
> + if ( n_ ) \
> + { \
> + if ( __builtin_constant_p(v) && !v_ ) \
> + __memzero(p_, n_); \
> + else \
> + memset(p_, v_, n_); \
> + } \
> + p_; \
> + })
>
> #endif
>
>
On 19.08.2022 09:59, Wei Chen wrote: >> -----Original Message----- >> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jan >> Beulich >> Sent: 2022年8月19日 15:50 >> >> - add parentheses where they were missing (MISRA) >> - make sure to evaluate also v exactly once (MISRA) >> - remove excess parentheses >> - rename local variables to not have leading underscores >> - apply Xen coding style >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> I wonder whether "if ( n_ )" is really helpful: It's extra code in all >> callers passing a non-constant size, just to cover a pretty rare case >> which memset() is required to deal with correctly anyway, and which > > What rare case we need to use n_ that can make memset happy? I'm afraid I don't understand the question. Jan > As your > comment, I read the code again, but it seems to work fine without n_. > > Cheers, > Wei Chen
Hi Jan, > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: 2022年8月19日 16:04 > To: Wei Chen <Wei.Chen@arm.com> > Cc: Julien Grall <julien@xen.org>; Stefano Stabellini > <sstabellini@kernel.org>; Volodymyr Babchuk <volodymyr_babchuk@epam.com>; > Bertrand Marquis <Bertrand.Marquis@arm.com>; xen- > devel@lists.xenproject.org > Subject: Re: [PATCH 2/2] Arm32: tidy the memset() macro > > On 19.08.2022 09:59, Wei Chen wrote: > >> -----Original Message----- > >> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of > Jan > >> Beulich > >> Sent: 2022年8月19日 15:50 > >> > >> - add parentheses where they were missing (MISRA) > >> - make sure to evaluate also v exactly once (MISRA) > >> - remove excess parentheses > >> - rename local variables to not have leading underscores > >> - apply Xen coding style > >> > >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > >> --- > >> I wonder whether "if ( n_ )" is really helpful: It's extra code in all > >> callers passing a non-constant size, just to cover a pretty rare case > >> which memset() is required to deal with correctly anyway, and which > > > > What rare case we need to use n_ that can make memset happy? > > I'm afraid I don't understand the question. > Sorry I didn't describe the problem clearly in the last email. You mentioned whether if (n_) is useful in your patch comments. I looked at the implementation of the current memset macro, and I didn't feel it was too useful. Then in the comments you mentioned that if (n_) is just to cover a very rare case. Does the rare case is memset(p, v, 0)? If this is the case, I agree with you, memset itself should be able to handle with size=0. Sorry again for confusing you! Thanks, Wei Chen > Jan > > > As your > > comment, I read the code again, but it seems to work fine without n_. > > > > Cheers, > > Wei Chen
On 19.08.2022 11:41, Wei Chen wrote: > Hi Jan, > >> -----Original Message----- >> From: Jan Beulich <jbeulich@suse.com> >> Sent: 2022年8月19日 16:04 >> To: Wei Chen <Wei.Chen@arm.com> >> Cc: Julien Grall <julien@xen.org>; Stefano Stabellini >> <sstabellini@kernel.org>; Volodymyr Babchuk <volodymyr_babchuk@epam.com>; >> Bertrand Marquis <Bertrand.Marquis@arm.com>; xen- >> devel@lists.xenproject.org >> Subject: Re: [PATCH 2/2] Arm32: tidy the memset() macro >> >> On 19.08.2022 09:59, Wei Chen wrote: >>>> -----Original Message----- >>>> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of >> Jan >>>> Beulich >>>> Sent: 2022年8月19日 15:50 >>>> >>>> - add parentheses where they were missing (MISRA) >>>> - make sure to evaluate also v exactly once (MISRA) >>>> - remove excess parentheses >>>> - rename local variables to not have leading underscores >>>> - apply Xen coding style >>>> >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>>> --- >>>> I wonder whether "if ( n_ )" is really helpful: It's extra code in all >>>> callers passing a non-constant size, just to cover a pretty rare case >>>> which memset() is required to deal with correctly anyway, and which >>> >>> What rare case we need to use n_ that can make memset happy? >> >> I'm afraid I don't understand the question. >> > > Sorry I didn't describe the problem clearly in the last email. You mentioned > whether if (n_) is useful in your patch comments. I looked at the implementation > of the current memset macro, and I didn't feel it was too useful. > > Then in the comments you mentioned that if (n_) is just to cover a very rare case. > Does the rare case is memset(p, v, 0)? Yes, albeit not in the form you've written it, but with the last argument being a variable which happens to be zero. With literal zero, the compiler would dead-code eliminate the construct anyway. Jan > If this is the case, I agree with you, > memset itself should be able to handle with size=0. > > Sorry again for confusing you! > > Thanks, > Wei Chen
Hi Jan, On 19/08/2022 08:50, Jan Beulich wrote: > - add parentheses where they were missing (MISRA) > - make sure to evaluate also v exactly once (MISRA) > - remove excess parentheses > - rename local variables to not have leading underscores > - apply Xen coding style This code has been taken from Linux. From you write above, I don't see any strong reason for us to modify it (even if it is small). Cheers, -- Julien Grall
On 19.08.2022 09:58, Julien Grall wrote: > On 19/08/2022 08:50, Jan Beulich wrote: >> - add parentheses where they were missing (MISRA) >> - make sure to evaluate also v exactly once (MISRA) >> - remove excess parentheses >> - rename local variables to not have leading underscores >> - apply Xen coding style > > This code has been taken from Linux. From you write above, I don't see > any strong reason for us to modify it (even if it is small). At least the MISRA issues want addressing, I suppose. Plus I wasn't able to spot the macro in Linux anymore (nor __memzero()), so to me there seemed to be little point to consider keeping anything "in sync" here. Jan
Hi Jan, On 19/08/2022 09:02, Jan Beulich wrote: > On 19.08.2022 09:58, Julien Grall wrote: >> On 19/08/2022 08:50, Jan Beulich wrote: >>> - add parentheses where they were missing (MISRA) >>> - make sure to evaluate also v exactly once (MISRA) >>> - remove excess parentheses >>> - rename local variables to not have leading underscores >>> - apply Xen coding style >> >> This code has been taken from Linux. From you write above, I don't see >> any strong reason for us to modify it (even if it is small). > > At least the MISRA issues want addressing, I suppose. Plus I wasn't > able to spot the macro in Linux anymore (nor __memzero()), so to me > there seemed to be little point to consider keeping anything "in sync" > here. I read the last part as we want a re-sync of the code (we haven't done one in the past couple of years). Cheers, -- Julien Grall
On 19.08.2022 10:06, Julien Grall wrote: > On 19/08/2022 09:02, Jan Beulich wrote: >> On 19.08.2022 09:58, Julien Grall wrote: >>> On 19/08/2022 08:50, Jan Beulich wrote: >>>> - add parentheses where they were missing (MISRA) >>>> - make sure to evaluate also v exactly once (MISRA) >>>> - remove excess parentheses >>>> - rename local variables to not have leading underscores >>>> - apply Xen coding style >>> >>> This code has been taken from Linux. From you write above, I don't see >>> any strong reason for us to modify it (even if it is small). >> >> At least the MISRA issues want addressing, I suppose. Plus I wasn't >> able to spot the macro in Linux anymore (nor __memzero()), so to me >> there seemed to be little point to consider keeping anything "in sync" >> here. > I read the last part as we want a re-sync of the code (we haven't done > one in the past couple of years). I'm afraid I'm now really confused: Which last part? I don't see how any of what I have said could be read that way. Quite the opposite: By stating that Linux doesn't have this macro anymore, isn't it quite clear that there's nothing to re-sync against? Jan
Hi Jan, On 19/08/2022 09:11, Jan Beulich wrote: > On 19.08.2022 10:06, Julien Grall wrote: >> On 19/08/2022 09:02, Jan Beulich wrote: >>> On 19.08.2022 09:58, Julien Grall wrote: >>>> On 19/08/2022 08:50, Jan Beulich wrote: >>>>> - add parentheses where they were missing (MISRA) >>>>> - make sure to evaluate also v exactly once (MISRA) >>>>> - remove excess parentheses >>>>> - rename local variables to not have leading underscores >>>>> - apply Xen coding style >>>> >>>> This code has been taken from Linux. From you write above, I don't see >>>> any strong reason for us to modify it (even if it is small). >>> >>> At least the MISRA issues want addressing, I suppose. Plus I wasn't >>> able to spot the macro in Linux anymore (nor __memzero()), so to me >>> there seemed to be little point to consider keeping anything "in sync" >>> here. >> I read the last part as we want a re-sync of the code (we haven't done >> one in the past couple of years). > > I'm afraid I'm now really confused: Which last part? I don't see how > any of what I have said could be read that way. Quite the opposite: > By stating that Linux doesn't have this macro anymore, isn't it quite > clear that there's nothing to re-sync against? Your view here if we will never re-sync the code. This is incorrect, we still want to keep it close so we can benefit from improvement in the Linux code. So if you start tweaking the code just for coding style purpose, it will just make it more difficult for us (I appreciate this is limited here). In this case, Linux has removed __memzero() is patch ff5fdafc9e97 "ARM: 8745/1: get rid of __memzero()" because the performance difference with memset() was limited. For Xen, I think we should also remove the function. With that, this patch becomes pointless. Cheers, -- Julien Grall
On 19.08.2022 10:24, Julien Grall wrote: > On 19/08/2022 09:11, Jan Beulich wrote: >> On 19.08.2022 10:06, Julien Grall wrote: >>> On 19/08/2022 09:02, Jan Beulich wrote: >>>> On 19.08.2022 09:58, Julien Grall wrote: >>>>> On 19/08/2022 08:50, Jan Beulich wrote: >>>>>> - add parentheses where they were missing (MISRA) >>>>>> - make sure to evaluate also v exactly once (MISRA) >>>>>> - remove excess parentheses >>>>>> - rename local variables to not have leading underscores >>>>>> - apply Xen coding style >>>>> >>>>> This code has been taken from Linux. From you write above, I don't see >>>>> any strong reason for us to modify it (even if it is small). >>>> >>>> At least the MISRA issues want addressing, I suppose. Plus I wasn't >>>> able to spot the macro in Linux anymore (nor __memzero()), so to me >>>> there seemed to be little point to consider keeping anything "in sync" >>>> here. >>> I read the last part as we want a re-sync of the code (we haven't done >>> one in the past couple of years). >> >> I'm afraid I'm now really confused: Which last part? I don't see how >> any of what I have said could be read that way. Quite the opposite: >> By stating that Linux doesn't have this macro anymore, isn't it quite >> clear that there's nothing to re-sync against? > Your view here if we will never re-sync the code. This is incorrect, we > still want to keep it close so we can benefit from improvement in the > Linux code. So if you start tweaking the code just for coding style > purpose, it will just make it more difficult for us (I appreciate this > is limited here). > > In this case, Linux has removed __memzero() is patch ff5fdafc9e97 "ARM: > 8745/1: get rid of __memzero()" because the performance difference with > memset() was limited. For Xen, I think we should also remove the function. > > With that, this patch becomes pointless. Of course. I could have named this as an alternative in a post-commit- message remark ... Looking forward to the re-syncing to be done then, at which point I'll happily drop this patch. Jan
Hi Jan, On 19/08/2022 09:31, Jan Beulich wrote: > On 19.08.2022 10:24, Julien Grall wrote: >> On 19/08/2022 09:11, Jan Beulich wrote: >>> On 19.08.2022 10:06, Julien Grall wrote: >>>> On 19/08/2022 09:02, Jan Beulich wrote: >>>>> On 19.08.2022 09:58, Julien Grall wrote: >>>>>> On 19/08/2022 08:50, Jan Beulich wrote: >>>>>>> - add parentheses where they were missing (MISRA) >>>>>>> - make sure to evaluate also v exactly once (MISRA) >>>>>>> - remove excess parentheses >>>>>>> - rename local variables to not have leading underscores >>>>>>> - apply Xen coding style >>>>>> >>>>>> This code has been taken from Linux. From you write above, I don't see >>>>>> any strong reason for us to modify it (even if it is small). >>>>> >>>>> At least the MISRA issues want addressing, I suppose. Plus I wasn't >>>>> able to spot the macro in Linux anymore (nor __memzero()), so to me >>>>> there seemed to be little point to consider keeping anything "in sync" >>>>> here. >>>> I read the last part as we want a re-sync of the code (we haven't done >>>> one in the past couple of years). >>> >>> I'm afraid I'm now really confused: Which last part? I don't see how >>> any of what I have said could be read that way. Quite the opposite: >>> By stating that Linux doesn't have this macro anymore, isn't it quite >>> clear that there's nothing to re-sync against? >> Your view here if we will never re-sync the code. This is incorrect, we >> still want to keep it close so we can benefit from improvement in the >> Linux code. So if you start tweaking the code just for coding style >> purpose, it will just make it more difficult for us (I appreciate this >> is limited here). >> >> In this case, Linux has removed __memzero() is patch ff5fdafc9e97 "ARM: >> 8745/1: get rid of __memzero()" because the performance difference with >> memset() was limited. For Xen, I think we should also remove the function. >> >> With that, this patch becomes pointless. > > Of course. I could have named this as an alternative in a post-commit- > message remark ... Looking forward to the re-syncing to be done then, > at which point I'll happily drop this patch. I will add it in my queue. I don't think this will reach 4.17 (unless someone else has time). Cheers, -- Julien Grall
© 2016 - 2026 Red Hat, Inc.