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

Amber, Kumar kumar.amber at intel.com
Wed Jul 7 10:03:52 UTC 2021


Hi Eelco,


I tried with the suggestion “zd” is deprecated and in place of it %"PRIdSIZE`` is mentioned which still causes build failure on non-ssl 32 bit builds.

Regards
Amber

From: Eelco Chaudron <echaudro at redhat.com>
Sent: Wednesday, July 7, 2021 3:02 PM
To: Van Haaren, Harry <harry.van.haaren at intel.com>
Cc: Amber, Kumar <kumar.amber at intel.com>; Ferriter, Cian <cian.ferriter at intel.com>; ovs-dev at openvswitch.org; fbl at sysclose.org; i.maximets at ovn.org; Stokes, Ian <ian.stokes at intel.com>
Subject: Re: [v6 00/11] MFEX Infrastructure + Optimizations


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

-----Original Message-----
From: Eelco Chaudron <echaudro at redhat.com<mailto:echaudro at redhat.com>>
Sent: Wednesday, July 7, 2021 9:35 AM
To: Amber, Kumar <kumar.amber at intel.com<mailto:kumar.amber at intel.com>>
Cc: Ferriter, Cian <cian.ferriter at intel.com<mailto:cian.ferriter at intel.com>>; ovs-dev at openvswitch.org<mailto:ovs-dev at openvswitch.org>;
fbl at sysclose.org<mailto:fbl at sysclose.org>; i.maximets at ovn.org<mailto:i.maximets at ovn.org>; Van Haaren, Harry
<harry.van.haaren at intel.com<mailto:harry.van.haaren at intel.com>>; Stokes, Ian <ian.stokes at intel.com<mailto: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>


More information about the dev mailing list