[ovs-dev] [v6 00/11] MFEX Infrastructure + Optimizations

[Eelco Chaudron]

On 7 Jul 2021, at 11:09, Van Haaren, Harry wrote:

>> -----Original Message-----
>> From: Eelco Chaudron <echaudro at redhat.com>
>> Sent: Wednesday, July 7, 2021 9:35 AM
>> To: Amber, Kumar <kumar.amber at intel.com>
>> Cc: Ferriter, Cian <cian.ferriter at intel.com>; 
>> ovs-dev at openvswitch.org;
>> fbl at sysclose.org; i.maximets at ovn.org; Van Haaren, Harry
>> <harry.van.haaren at intel.com>; Stokes, Ian <ian.stokes at intel.com>
>> Subject: Re: [v6 00/11] MFEX Infrastructure + Optimizations
>>
>>
>>
>> On 6 Jul 2021, at 17:06, Amber, Kumar wrote:
>>
>>> Hi Eelco ,
>>>
>>>
>>> Here is the diff  vor v6 vs v5 :
>>>
>>> Patch 1 :
>>>
>>> diff --git a/lib/dpif-netdev-private-extract.c 
>>> b/lib/dpif-netdev-private-extract.c
>>> index 1aebf3656d..4987d628a4 100644
>>> --- a/lib/dpif-netdev-private-extract.c
>>> +++ b/lib/dpif-netdev-private-extract.c
>>> @@ -233,7 +233,7 @@ dpif_miniflow_extract_autovalidator(struct
>> dp_packet_batch *packets,
>>>                                      uint32_t keys_size, odp_port_t 
>>> in_port,
>>>                                      struct dp_netdev_pmd_thread 
>>> *pmd_handle)
>>>  {
>>> -    const size_t cnt = dp_packet_batch_size(packets);
>>> +    const uint32_t cnt = dp_packet_batch_size(packets);
>>>      uint16_t good_l2_5_ofs[NETDEV_MAX_BURST];
>>>      uint16_t good_l3_ofs[NETDEV_MAX_BURST];
>>>      uint16_t good_l4_ofs[NETDEV_MAX_BURST];
>>> @@ -247,7 +247,7 @@ dpif_miniflow_extract_autovalidator(struct
>> dp_packet_batch *packets,
>>>          atomic_uintptr_t *pmd_func = (void 
>>> *)&pmd->miniflow_extract_opt;
>>>          atomic_store_relaxed(pmd_func, (uintptr_t) default_func);
>>>          VLOG_ERR("Invalid key size supplied, Key_size: %d less 
>>> than"
>>> -                 "batch_size: %ld", keys_size, cnt);
>>> +                 "batch_size: %d", keys_size, cnt);
>>
>> What was the reason for changing this size_t to uint32_t? Is see 
>> other instances
>> where %ld is used for logging?
>> And other functions like dp_netdev_run_meter() have it as a size_t?
>
> The reason to change this is because 32-bit builds were breaking due 
> to incorrect
> format-specifier in the printf. Root cause is because size_t requires 
> different printf
> format specifier based on 32 or 64 bit arch.
>
> (As you likely know, size_t is to describe objects in memory, or the 
> return of sizeof operator.
> Because 32-bit and 64-bit can have different amounts of memory, size_t 
> can be "unsigned int"
> or "unsigned long long").
>
> It does not make sense to me to use a type of variable that changes 
> width based on
> architecture to count batch size (a value from 0 to 32).
>
> Simplicity and obvious-ness is nice, and a uint32_t is always exactly 
> what you read it to be,
> and %d will always be correct for uint32_t regardless of 32 or 64 bit.
>
> We should not change this back to the more complex and error-prone 
> "size_t", uint32_t is better.

I don't think it’s more error-prone if the right type qualifier is 
used, i.e. %zd. See also the coding style document, so I would suggest 
changing it to:

@@ -233,7 +233,7 @@ dpif_miniflow_extract_autovalidator(struct 
dp_packet_batch *packets,
                                      uint32_t keys_size, odp_port_t 
in_port,
                                      struct dp_netdev_pmd_thread 
*pmd_handle)
  {
-    const uint32_t cnt = dp_packet_batch_size(packets);
+    const size_t cnt = dp_packet_batch_size(packets);
      uint16_t good_l2_5_ofs[NETDEV_MAX_BURST];
      uint16_t good_l3_ofs[NETDEV_MAX_BURST];
      uint16_t good_l4_ofs[NETDEV_MAX_BURST];
@@ -247,7 +247,7 @@ dpif_miniflow_extract_autovalidator(struct 
dp_packet_batch *packets,
          atomic_uintptr_t *pmd_func = (void 
*)&pmd->miniflow_extract_opt;
          atomic_store_relaxed(pmd_func, (uintptr_t) default_func);
          VLOG_ERR("Invalid key size supplied, Key_size: %d less than"
-                 "batch_size: %d", keys_size, cnt);
+                 "batch_size: %"PRIdSIZE, keys_size, cnt);
          return 0;
      }



> <snip>