Hi Edward / Andrew I like your suggestions and applied with [v5] net/virtio: fix wrong variable assignment in helper macro V4 had a extra line typos., V5 is tested compiled and pushed carefully Thanks!
Regards Vipul -----Original Message----- From: Edward Makarov [mailto:maka...@kraftway.ru] Sent: Sunday, 30 August, 2020 3:48 To: Andrew Rybchenko <arybche...@solarflare.com>; Vipul Ashri <vipul.as...@oracle.com>; dev@dpdk.org Cc: chenbo....@intel.com; zhihong.w...@intel.com; maxime.coque...@redhat.com; sta...@dpdk.org Subject: Re: [dpdk-dev] [PATCH] net/virtio: fix wrong variable assignment in helper macro On 8/29/20 2:22 PM, Andrew Rybchenko wrote: > On 8/14/20 12:21 PM, Vipul Ashri wrote: >> Inside Macro ASSIGN_UNLESS_EQUAL(var, val), assignment to var is >> always failing as assignment done using var_ having local scope only. >> This leads to TX packets not going out and found broken due to >> cleanup malfunctioning. This patch fixes the wrong variable assignment. >> >> Fixes: 57f90f894588 ("net/virtio: reuse packed ring functions") >> Cc: sta...@dpdk.org >> >> Signed-off-by: Vipul Ashri <vipul.as...@oracle.com> >> --- >> drivers/net/virtio/virtqueue.h | 6 ++---- >> 1 file changed, 2 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/net/virtio/virtqueue.h >> b/drivers/net/virtio/virtqueue.h index 105a9c00c..20c95471e 100644 >> --- a/drivers/net/virtio/virtqueue.h >> +++ b/drivers/net/virtio/virtqueue.h >> @@ -607,10 +607,8 @@ virtqueue_notify(struct virtqueue *vq) >> >> /* avoid write operation when necessary, to lessen cache issues */ >> #define ASSIGN_UNLESS_EQUAL(var, val) do { \ >> - typeof(var) var_ = (var); \ >> - typeof(val) val_ = (val); \ >> - if ((var_) != (val_)) \ >> - (var_) = (val_); \ >> + if ((var) != (val)) \ >> + (var) = (val); \ > > Good catch. As I understand the old code tries to avoid processing of > var and val expressions twice. It looks it could be kept for val at > least. Just keep if condition as in old code and fix the last line > above: > (var) = val_; > Since var_ and val_are local variables there is no necessity to > enclose it in parenthesis (but does not harm if done). > var_ may be really removed since since resulting code will use it only > once. I think there is a solution to avoid multiple evaluations of parameters: // var is definitely an lvalue, so its address can definitely be taken #define ASSIGN_UNLESS_EQUAL(var, val) do { \ typeof(var) *const var_ = &(var); \ typeof(val) const val_ = (val); \ if (*var_ != val_) \ *var_ = val_; \ } while (0) The solution relies on the compiler optimizer. Here is the comparison of what the variants are compiled into: https://urldefense.com/v3/__https://godbolt.org/z/nnvq5q__;!!GqivPVa7Brio!MJZCNAgcCHB5A_T1mI2aA-2F9wvqh_WOfzkeN0IbDnsZSlNIPWqDF0b4YDmTc19HcA$
