[Xen-devel,RFC,v4,2/8] xen/bitops: Rename LOG_2 to ilog2

Message ID 20171219031703.23420-3-sameer.goel@linaro.org
State Superseded
Headers show
Series
  • SMMUv3 driver
Related show

Commit Message

Sameer Goel Dec. 19, 2017, 3:16 a.m.
Changing the name of the macro from LOG_2 to ilog2.This makes the function name
similar to its Linux counterpart. Since, this is not used in mutiple places, so the code churn is minimal.
This change helps in porting unchanged code from Linux.

Signed-off-by: Sameer Goel <sameer.goel@linaro.org>
---
 xen/arch/x86/x86_64/asm-offsets.c | 2 +-
 xen/include/xen/bitops.h          | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Comments

Julien Grall Jan. 23, 2018, 11:36 a.m. | #1
Hi Sameer,

On 19/12/17 03:16, Sameer Goel wrote:
> Changing the name of the macro from LOG_2 to ilog2.This makes the function name
> similar to its Linux counterpart. Since, this is not used in mutiple places, so the code churn is minimal.

s/mutiple/multiple/

> This change helps in porting unchanged code from Linux.

Can you wrap the commit message correctly?

Cheers,

> 
> Signed-off-by: Sameer Goel <sameer.goel@linaro.org>
> ---
>   xen/arch/x86/x86_64/asm-offsets.c | 2 +-
>   xen/include/xen/bitops.h          | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/x86_64/asm-offsets.c b/xen/arch/x86/x86_64/asm-offsets.c
> index e136af6b99..4bccbc9bdf 100644
> --- a/xen/arch/x86/x86_64/asm-offsets.c
> +++ b/xen/arch/x86/x86_64/asm-offsets.c
> @@ -157,7 +157,7 @@ void __dummy__(void)
>       BLANK();
>   #endif
>   
> -    DEFINE(IRQSTAT_shift, LOG_2(sizeof(irq_cpustat_t)));
> +    DEFINE(IRQSTAT_shift, ilog2(sizeof(irq_cpustat_t)));
>       OFFSET(IRQSTAT_softirq_pending, irq_cpustat_t, __softirq_pending);
>       BLANK();
>   
> diff --git a/xen/include/xen/bitops.h b/xen/include/xen/bitops.h
> index e2019b02a3..a103e49089 100644
> --- a/xen/include/xen/bitops.h
> +++ b/xen/include/xen/bitops.h
> @@ -223,7 +223,7 @@ static inline __u32 ror32(__u32 word, unsigned int shift)
>   #define __L4(_x)  (((_x) & 0x0000000c) ? ( 2 + __L2( (_x)>> 2)) : __L2( _x))
>   #define __L8(_x)  (((_x) & 0x000000f0) ? ( 4 + __L4( (_x)>> 4)) : __L4( _x))
>   #define __L16(_x) (((_x) & 0x0000ff00) ? ( 8 + __L8( (_x)>> 8)) : __L8( _x))
> -#define LOG_2(_x) (((_x) & 0xffff0000) ? (16 + __L16((_x)>>16)) : __L16(_x))
> +#define ilog2(_x) (((_x) & 0xffff0000) ? (16 + __L16((_x)>>16)) : __L16(_x))
>   
>   /**
>    * for_each_set_bit - iterate over every set bit in a memory region
>
Roger Pau Monné Jan. 23, 2018, 11:39 a.m. | #2
On Mon, Dec 18, 2017 at 08:16:57PM -0700, Sameer Goel wrote:
> diff --git a/xen/include/xen/bitops.h b/xen/include/xen/bitops.h
> index e2019b02a3..a103e49089 100644
> --- a/xen/include/xen/bitops.h
> +++ b/xen/include/xen/bitops.h
> @@ -223,7 +223,7 @@ static inline __u32 ror32(__u32 word, unsigned int shift)
>  #define __L4(_x)  (((_x) & 0x0000000c) ? ( 2 + __L2( (_x)>> 2)) : __L2( _x))
>  #define __L8(_x)  (((_x) & 0x000000f0) ? ( 4 + __L4( (_x)>> 4)) : __L4( _x))
>  #define __L16(_x) (((_x) & 0x0000ff00) ? ( 8 + __L8( (_x)>> 8)) : __L8( _x))
> -#define LOG_2(_x) (((_x) & 0xffff0000) ? (16 + __L16((_x)>>16)) : __L16(_x))
> +#define ilog2(_x) (((_x) & 0xffff0000) ? (16 + __L16((_x)>>16)) : __L16(_x))

Er, why not add a:

#define ilog2 LOG_2

On the code that you have to import?

Roger.
Julien Grall Jan. 23, 2018, 11:44 a.m. | #3
Hi Roger,

On 23/01/18 11:39, Roger Pau Monné wrote:
> On Mon, Dec 18, 2017 at 08:16:57PM -0700, Sameer Goel wrote:
>> diff --git a/xen/include/xen/bitops.h b/xen/include/xen/bitops.h
>> index e2019b02a3..a103e49089 100644
>> --- a/xen/include/xen/bitops.h
>> +++ b/xen/include/xen/bitops.h
>> @@ -223,7 +223,7 @@ static inline __u32 ror32(__u32 word, unsigned int shift)
>>   #define __L4(_x)  (((_x) & 0x0000000c) ? ( 2 + __L2( (_x)>> 2)) : __L2( _x))
>>   #define __L8(_x)  (((_x) & 0x000000f0) ? ( 4 + __L4( (_x)>> 4)) : __L4( _x))
>>   #define __L16(_x) (((_x) & 0x0000ff00) ? ( 8 + __L8( (_x)>> 8)) : __L8( _x))
>> -#define LOG_2(_x) (((_x) & 0xffff0000) ? (16 + __L16((_x)>>16)) : __L16(_x))
>> +#define ilog2(_x) (((_x) & 0xffff0000) ? (16 + __L16((_x)>>16)) : __L16(_x))
> 
> Er, why not add a:
> 
> #define ilog2 LOG_2
> 
> On the code that you have to import?

There is exactly on caller of LOG_2. So what is the benefits to provide 
2 names? More that I would expect ilog2 to be used in any code coming 
from Linux.

Cheers,

> 
> Roger.
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
>
Roger Pau Monné Jan. 23, 2018, 12:10 p.m. | #4
On Tue, Jan 23, 2018 at 11:44:30AM +0000, Julien Grall wrote:
> Hi Roger,
> 
> On 23/01/18 11:39, Roger Pau Monné wrote:
> > On Mon, Dec 18, 2017 at 08:16:57PM -0700, Sameer Goel wrote:
> > > diff --git a/xen/include/xen/bitops.h b/xen/include/xen/bitops.h
> > > index e2019b02a3..a103e49089 100644
> > > --- a/xen/include/xen/bitops.h
> > > +++ b/xen/include/xen/bitops.h
> > > @@ -223,7 +223,7 @@ static inline __u32 ror32(__u32 word, unsigned int shift)
> > >   #define __L4(_x)  (((_x) & 0x0000000c) ? ( 2 + __L2( (_x)>> 2)) : __L2( _x))
> > >   #define __L8(_x)  (((_x) & 0x000000f0) ? ( 4 + __L4( (_x)>> 4)) : __L4( _x))
> > >   #define __L16(_x) (((_x) & 0x0000ff00) ? ( 8 + __L8( (_x)>> 8)) : __L8( _x))
> > > -#define LOG_2(_x) (((_x) & 0xffff0000) ? (16 + __L16((_x)>>16)) : __L16(_x))
> > > +#define ilog2(_x) (((_x) & 0xffff0000) ? (16 + __L16((_x)>>16)) : __L16(_x))
> > 
> > Er, why not add a:
> > 
> > #define ilog2 LOG_2
> > 
> > On the code that you have to import?
> 
> There is exactly on caller of LOG_2. So what is the benefits to provide 2
> names? More that I would expect ilog2 to be used in any code coming from
> Linux.

I don't think we should be renaming macros just because we want to
import verbatim code from Linux or anywhere else, I would rather add a
linuxkpi.h or similar in order to do the translation, but that's just
my opinion.

Roger.
Julien Grall Jan. 23, 2018, 12:17 p.m. | #5
Hi Roger,

On 23/01/18 12:10, Roger Pau Monné wrote:
> On Tue, Jan 23, 2018 at 11:44:30AM +0000, Julien Grall wrote:
>> Hi Roger,
>>
>> On 23/01/18 11:39, Roger Pau Monné wrote:
>>> On Mon, Dec 18, 2017 at 08:16:57PM -0700, Sameer Goel wrote:
>>>> diff --git a/xen/include/xen/bitops.h b/xen/include/xen/bitops.h
>>>> index e2019b02a3..a103e49089 100644
>>>> --- a/xen/include/xen/bitops.h
>>>> +++ b/xen/include/xen/bitops.h
>>>> @@ -223,7 +223,7 @@ static inline __u32 ror32(__u32 word, unsigned int shift)
>>>>    #define __L4(_x)  (((_x) & 0x0000000c) ? ( 2 + __L2( (_x)>> 2)) : __L2( _x))
>>>>    #define __L8(_x)  (((_x) & 0x000000f0) ? ( 4 + __L4( (_x)>> 4)) : __L4( _x))
>>>>    #define __L16(_x) (((_x) & 0x0000ff00) ? ( 8 + __L8( (_x)>> 8)) : __L8( _x))
>>>> -#define LOG_2(_x) (((_x) & 0xffff0000) ? (16 + __L16((_x)>>16)) : __L16(_x))
>>>> +#define ilog2(_x) (((_x) & 0xffff0000) ? (16 + __L16((_x)>>16)) : __L16(_x))
>>>
>>> Er, why not add a:
>>>
>>> #define ilog2 LOG_2
>>>
>>> On the code that you have to import?
>>
>> There is exactly on caller of LOG_2. So what is the benefits to provide 2
>> names? More that I would expect ilog2 to be used in any code coming from
>> Linux.
> 
> I don't think we should be renaming macros just because we want to
> import verbatim code from Linux or anywhere else, I would rather add a
> linuxkpi.h or similar in order to do the translation, but that's just
> my opinion.

I would have agreed if there was 10-30 callsite in Xen code. For 1 
callsite it sounds like a bit overkill to request to have 2 different 
name. You can see this as renaming very similar to series that rename field.

Cheers,

Patch

diff --git a/xen/arch/x86/x86_64/asm-offsets.c b/xen/arch/x86/x86_64/asm-offsets.c
index e136af6b99..4bccbc9bdf 100644
--- a/xen/arch/x86/x86_64/asm-offsets.c
+++ b/xen/arch/x86/x86_64/asm-offsets.c
@@ -157,7 +157,7 @@  void __dummy__(void)
     BLANK();
 #endif
 
-    DEFINE(IRQSTAT_shift, LOG_2(sizeof(irq_cpustat_t)));
+    DEFINE(IRQSTAT_shift, ilog2(sizeof(irq_cpustat_t)));
     OFFSET(IRQSTAT_softirq_pending, irq_cpustat_t, __softirq_pending);
     BLANK();
 
diff --git a/xen/include/xen/bitops.h b/xen/include/xen/bitops.h
index e2019b02a3..a103e49089 100644
--- a/xen/include/xen/bitops.h
+++ b/xen/include/xen/bitops.h
@@ -223,7 +223,7 @@  static inline __u32 ror32(__u32 word, unsigned int shift)
 #define __L4(_x)  (((_x) & 0x0000000c) ? ( 2 + __L2( (_x)>> 2)) : __L2( _x))
 #define __L8(_x)  (((_x) & 0x000000f0) ? ( 4 + __L4( (_x)>> 4)) : __L4( _x))
 #define __L16(_x) (((_x) & 0x0000ff00) ? ( 8 + __L8( (_x)>> 8)) : __L8( _x))
-#define LOG_2(_x) (((_x) & 0xffff0000) ? (16 + __L16((_x)>>16)) : __L16(_x))
+#define ilog2(_x) (((_x) & 0xffff0000) ? (16 + __L16((_x)>>16)) : __L16(_x))
 
 /**
  * for_each_set_bit - iterate over every set bit in a memory region