diff mbox series

[1/1] IB: rxe: replace ARRAYSIZE with RXE_NUM_OF_COUNTERS

Message ID 20181215080952.15762-1-yanjun.zhu@oracle.com (mailing list archive)
State Changes Requested
Headers show
Series [1/1] IB: rxe: replace ARRAYSIZE with RXE_NUM_OF_COUNTERS | expand

Commit Message

Zhu Yanjun Dec. 15, 2018, 8:09 a.m. UTC 
RXE_NUM_OF_COUNTERS is the number of counters. So it can be used
directly.

Signed-off-by: Zhu Yanjun <yanjun.zhu@oracle.com>
---
 drivers/infiniband/sw/rxe/rxe_hw_counters.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Comments

Bart Van Assche Dec. 15, 2018, 3:43 p.m. UTC | #1 
On 12/15/18 12:09 AM, Zhu Yanjun wrote:
> RXE_NUM_OF_COUNTERS is the number of counters. So it can be used
> directly.
> 
> Signed-off-by: Zhu Yanjun <yanjun.zhu@oracle.com>
> ---
>   drivers/infiniband/sw/rxe/rxe_hw_counters.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/infiniband/sw/rxe/rxe_hw_counters.c b/drivers/infiniband/sw/rxe/rxe_hw_counters.c
> index 6aeb7a165e46..8df4b1592d6d 100644
> --- a/drivers/infiniband/sw/rxe/rxe_hw_counters.c
> +++ b/drivers/infiniband/sw/rxe/rxe_hw_counters.c
> @@ -58,21 +58,20 @@ int rxe_ib_get_hw_stats(struct ib_device *ibdev,
>   	if (!port || !stats)
>   		return -EINVAL;
>   
> -	for (cnt = 0; cnt  < ARRAY_SIZE(rxe_counter_name); cnt++)
> +	for (cnt = 0; cnt  < RXE_NUM_OF_COUNTERS; cnt++)
>   		stats->value[cnt] = dev->stats_counters[cnt];
>   
> -	return ARRAY_SIZE(rxe_counter_name);
> +	return RXE_NUM_OF_COUNTERS;
>   }
>   
>   struct rdma_hw_stats *rxe_ib_alloc_hw_stats(struct ib_device *ibdev,
>   					    u8 port_num)
>   {
> -	BUILD_BUG_ON(ARRAY_SIZE(rxe_counter_name) != RXE_NUM_OF_COUNTERS);
>   	/* We support only per port stats */
>   	if (!port_num)
>   		return NULL;
>   
>   	return rdma_alloc_hw_stats_struct(rxe_counter_name,
> -					  ARRAY_SIZE(rxe_counter_name),
> +					  RXE_NUM_OF_COUNTERS,
>   					  RDMA_HW_STATS_DEFAULT_LIFESPAN);
>   }

Have you considered to make the opposite change, namely to remove the 
RXE_NUM_OF_COUNTERS constant and to use ARRAY_SIZE(rxe_counter_name) 
everywhere instead?

Thanks,

Bart.
Zhu Yanjun Dec. 16, 2018, 1:30 a.m. UTC | #2 
On 2018/12/15 23:43, Bart Van Assche wrote:
> On 12/15/18 12:09 AM, Zhu Yanjun wrote:
>> RXE_NUM_OF_COUNTERS is the number of counters. So it can be used
>> directly.
>>
>> Signed-off-by: Zhu Yanjun <yanjun.zhu@oracle.com>
>> ---
>>   drivers/infiniband/sw/rxe/rxe_hw_counters.c | 7 +++----
>>   1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/infiniband/sw/rxe/rxe_hw_counters.c 
>> b/drivers/infiniband/sw/rxe/rxe_hw_counters.c
>> index 6aeb7a165e46..8df4b1592d6d 100644
>> --- a/drivers/infiniband/sw/rxe/rxe_hw_counters.c
>> +++ b/drivers/infiniband/sw/rxe/rxe_hw_counters.c
>> @@ -58,21 +58,20 @@ int rxe_ib_get_hw_stats(struct ib_device *ibdev,
>>       if (!port || !stats)
>>           return -EINVAL;
>>   -    for (cnt = 0; cnt  < ARRAY_SIZE(rxe_counter_name); cnt++)
>> +    for (cnt = 0; cnt  < RXE_NUM_OF_COUNTERS; cnt++)
>>           stats->value[cnt] = dev->stats_counters[cnt];
>>   -    return ARRAY_SIZE(rxe_counter_name);
>> +    return RXE_NUM_OF_COUNTERS;
>>   }
>>     struct rdma_hw_stats *rxe_ib_alloc_hw_stats(struct ib_device *ibdev,
>>                           u8 port_num)
>>   {
>> -    BUILD_BUG_ON(ARRAY_SIZE(rxe_counter_name) != RXE_NUM_OF_COUNTERS);
>>       /* We support only per port stats */
>>       if (!port_num)
>>           return NULL;
>>         return rdma_alloc_hw_stats_struct(rxe_counter_name,
>> -                      ARRAY_SIZE(rxe_counter_name),
>> +                      RXE_NUM_OF_COUNTERS,
>>                         RDMA_HW_STATS_DEFAULT_LIFESPAN);
>>   }
>
> Have you considered to make the opposite change, namely to remove the 
> RXE_NUM_OF_COUNTERS constant and to use ARRAY_SIZE(rxe_counter_name) 
> everywhere instead?

Hi, Bart

Interesting. Can you give me a reason? Why ARRAY_SIZE is more important 
than RXE_NUM_OF_COUNTERS?

Zhu Yanjun

>
> Thanks,
>
> Bart.
>
>
Bart Van Assche Dec. 16, 2018, 2:16 a.m. UTC | #3 
On 12/15/18 5:30 PM, Yanjun Zhu wrote:
> 
> On 2018/12/15 23:43, Bart Van Assche wrote:
>> On 12/15/18 12:09 AM, Zhu Yanjun wrote:
>>> RXE_NUM_OF_COUNTERS is the number of counters. So it can be used
>>> directly.
>>>
>>> Signed-off-by: Zhu Yanjun <yanjun.zhu@oracle.com>
>>> ---
>>>   drivers/infiniband/sw/rxe/rxe_hw_counters.c | 7 +++----
>>>   1 file changed, 3 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/infiniband/sw/rxe/rxe_hw_counters.c 
>>> b/drivers/infiniband/sw/rxe/rxe_hw_counters.c
>>> index 6aeb7a165e46..8df4b1592d6d 100644
>>> --- a/drivers/infiniband/sw/rxe/rxe_hw_counters.c
>>> +++ b/drivers/infiniband/sw/rxe/rxe_hw_counters.c
>>> @@ -58,21 +58,20 @@ int rxe_ib_get_hw_stats(struct ib_device *ibdev,
>>>       if (!port || !stats)
>>>           return -EINVAL;
>>>   -    for (cnt = 0; cnt  < ARRAY_SIZE(rxe_counter_name); cnt++)
>>> +    for (cnt = 0; cnt  < RXE_NUM_OF_COUNTERS; cnt++)
>>>           stats->value[cnt] = dev->stats_counters[cnt];
>>>   -    return ARRAY_SIZE(rxe_counter_name);
>>> +    return RXE_NUM_OF_COUNTERS;
>>>   }
>>>     struct rdma_hw_stats *rxe_ib_alloc_hw_stats(struct ib_device *ibdev,
>>>                           u8 port_num)
>>>   {
>>> -    BUILD_BUG_ON(ARRAY_SIZE(rxe_counter_name) != RXE_NUM_OF_COUNTERS);
>>>       /* We support only per port stats */
>>>       if (!port_num)
>>>           return NULL;
>>>         return rdma_alloc_hw_stats_struct(rxe_counter_name,
>>> -                      ARRAY_SIZE(rxe_counter_name),
>>> +                      RXE_NUM_OF_COUNTERS,
>>>                         RDMA_HW_STATS_DEFAULT_LIFESPAN);
>>>   }
>>
>> Have you considered to make the opposite change, namely to remove the 
>> RXE_NUM_OF_COUNTERS constant and to use ARRAY_SIZE(rxe_counter_name) 
>> everywhere instead?
> 
> Interesting. Can you give me a reason? Why ARRAY_SIZE is more important 
> than RXE_NUM_OF_COUNTERS?

Hi Yanjun,

Using ARRAY_SIZE() makes it easy to verify correctness of code that uses 
that macro because it is guaranteed that the ARRAY_SIZE() result matches 
the array size. When using a macro however an additional lookup is 
required to verify whether the value of that macro really matches the 
array size. I think in the Linux kernel using ARRAY_SIZE() is preferred 
instead of introducing an explicit define for the array size.

Bart.
Zhu Yanjun Dec. 16, 2018, 10:58 a.m. UTC | #4 
On 2018/12/16 10:16, Bart Van Assche wrote:
> On 12/15/18 5:30 PM, Yanjun Zhu wrote:
>>
>> On 2018/12/15 23:43, Bart Van Assche wrote:
>>> On 12/15/18 12:09 AM, Zhu Yanjun wrote:
>>>> RXE_NUM_OF_COUNTERS is the number of counters. So it can be used
>>>> directly.
>>>>
>>>> Signed-off-by: Zhu Yanjun <yanjun.zhu@oracle.com>
>>>> ---
>>>>   drivers/infiniband/sw/rxe/rxe_hw_counters.c | 7 +++----
>>>>   1 file changed, 3 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/infiniband/sw/rxe/rxe_hw_counters.c 
>>>> b/drivers/infiniband/sw/rxe/rxe_hw_counters.c
>>>> index 6aeb7a165e46..8df4b1592d6d 100644
>>>> --- a/drivers/infiniband/sw/rxe/rxe_hw_counters.c
>>>> +++ b/drivers/infiniband/sw/rxe/rxe_hw_counters.c
>>>> @@ -58,21 +58,20 @@ int rxe_ib_get_hw_stats(struct ib_device *ibdev,
>>>>       if (!port || !stats)
>>>>           return -EINVAL;
>>>>   -    for (cnt = 0; cnt  < ARRAY_SIZE(rxe_counter_name); cnt++)
>>>> +    for (cnt = 0; cnt  < RXE_NUM_OF_COUNTERS; cnt++)
>>>>           stats->value[cnt] = dev->stats_counters[cnt];
>>>>   -    return ARRAY_SIZE(rxe_counter_name);
>>>> +    return RXE_NUM_OF_COUNTERS;
>>>>   }
>>>>     struct rdma_hw_stats *rxe_ib_alloc_hw_stats(struct ib_device 
>>>> *ibdev,
>>>>                           u8 port_num)
>>>>   {
>>>> -    BUILD_BUG_ON(ARRAY_SIZE(rxe_counter_name) != 
>>>> RXE_NUM_OF_COUNTERS);
>>>>       /* We support only per port stats */
>>>>       if (!port_num)
>>>>           return NULL;
>>>>         return rdma_alloc_hw_stats_struct(rxe_counter_name,
>>>> -                      ARRAY_SIZE(rxe_counter_name),
>>>> +                      RXE_NUM_OF_COUNTERS,
>>>>                         RDMA_HW_STATS_DEFAULT_LIFESPAN);
>>>>   }
>>>
>>> Have you considered to make the opposite change, namely to remove 
>>> the RXE_NUM_OF_COUNTERS constant and to use 
>>> ARRAY_SIZE(rxe_counter_name) everywhere instead?
>>
>> Interesting. Can you give me a reason? Why ARRAY_SIZE is more 
>> important than RXE_NUM_OF_COUNTERS?
>
> Hi Yanjun,
>
> Using ARRAY_SIZE() makes it easy to verify correctness of code that 
> uses that macro because it is guaranteed that the ARRAY_SIZE() result 
> matches the array size. When using a macro however an additional 
> lookup is required to verify whether the value of that macro really 
> matches the array size. I think in the Linux kernel using ARRAY_SIZE() 
> is preferred instead of introducing an explicit define for the array 
> size.

Thanks. Your concerns sound reasonable. I will send V2 soon.


Zhu Yanjun

>
> Bart.
diff mbox series

Patch

diff --git a/drivers/infiniband/sw/rxe/rxe_hw_counters.c b/drivers/infiniband/sw/rxe/rxe_hw_counters.c
index 6aeb7a165e46..8df4b1592d6d 100644
--- a/drivers/infiniband/sw/rxe/rxe_hw_counters.c
+++ b/drivers/infiniband/sw/rxe/rxe_hw_counters.c
@@ -58,21 +58,20 @@  int rxe_ib_get_hw_stats(struct ib_device *ibdev,
 	if (!port || !stats)
 		return -EINVAL;
 
-	for (cnt = 0; cnt  < ARRAY_SIZE(rxe_counter_name); cnt++)
+	for (cnt = 0; cnt  < RXE_NUM_OF_COUNTERS; cnt++)
 		stats->value[cnt] = dev->stats_counters[cnt];
 
-	return ARRAY_SIZE(rxe_counter_name);
+	return RXE_NUM_OF_COUNTERS;
 }
 
 struct rdma_hw_stats *rxe_ib_alloc_hw_stats(struct ib_device *ibdev,
 					    u8 port_num)
 {
-	BUILD_BUG_ON(ARRAY_SIZE(rxe_counter_name) != RXE_NUM_OF_COUNTERS);
 	/* We support only per port stats */
 	if (!port_num)
 		return NULL;
 
 	return rdma_alloc_hw_stats_struct(rxe_counter_name,
-					  ARRAY_SIZE(rxe_counter_name),
+					  RXE_NUM_OF_COUNTERS,
 					  RDMA_HW_STATS_DEFAULT_LIFESPAN);
 }