On 7 Jul 2021, at 12:13, Van Haaren, Harry wrote:
> Hi All, > > This thread has dissolved into unnecessary time-wasting on nitpick changes. > There is no > technical issue with uint32_t, so this patch remains as is, and this should > be accepted for merge. > > If you feel differently, reply to this with a detailed description of a > genuine technical bug. Reviews are not only about technical correctness but also about coding style and consistency. In this case the dp_packet_batch_size() API returns a size_t, so we should try to use it. But I leave it to the maintainers to decide if they accept this as is, or not :) //Eelco > Regards, -Harry > > > From: Amber, Kumar <kumar.am...@intel.com> > Sent: Wednesday, July 7, 2021 11:04 AM > To: Eelco Chaudron <echau...@redhat.com>; Van Haaren, Harry > <harry.van.haa...@intel.com> > Cc: Ferriter, Cian <cian.ferri...@intel.com>; ovs-dev@openvswitch.org; > f...@sysclose.org; i.maxim...@ovn.org; Stokes, Ian <ian.sto...@intel.com> > Subject: RE: [v6 00/11] MFEX Infrastructure + Optimizations > > 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 <echau...@redhat.com<mailto:echau...@redhat.com>> > Sent: Wednesday, July 7, 2021 3:02 PM > To: Van Haaren, Harry > <harry.van.haa...@intel.com<mailto:harry.van.haa...@intel.com>> > Cc: Amber, Kumar <kumar.am...@intel.com<mailto:kumar.am...@intel.com>> ; > Ferriter, Cian <cian.ferri...@intel.com<mailto:cian.ferri...@intel.com>> ; > ovs-dev@openvswitch.org<mailto:ovs-dev@openvswitch.org> ; > f...@sysclose.org<mailto:f...@sysclose.org> ; > i.maxim...@ovn.org<mailto:i.maxim...@ovn.org> ; Stokes, Ian > <ian.sto...@intel.com<mailto:ian.sto...@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 <echau...@redhat.com<mailto:echau...@redhat.com>> > Sent: Wednesday, July 7, 2021 9:35 AM > To: Amber, Kumar <kumar.am...@intel.com<mailto:kumar.am...@intel.com>> > Cc: Ferriter, Cian <cian.ferri...@intel.com<mailto:cian.ferri...@intel.com>> > ; ovs-dev@openvswitch.org<mailto:ovs-dev@openvswitch.org> ; > f...@sysclose.org<mailto:f...@sysclose.org> ; > i.maxim...@ovn.org<mailto:i.maxim...@ovn.org> ; Van Haaren, Harry > <harry.van.haa...@intel.com<mailto:harry.van.haa...@intel.com>> ; Stokes, Ian > <ian.sto...@intel.com<mailto:ian.sto...@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> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
