* [PATCH] lib/cryptodev: fix assertion to remove GCC compilation warning
@ 2023-05-22 18:49 Kamil Godzwon
2023-05-22 19:04 ` [PATCH v2] " Kamil Godzwon
0 siblings, 1 reply; 6+ messages in thread
From: Kamil Godzwon @ 2023-05-22 18:49 UTC (permalink / raw)
To: dev; +Cc: Akhil Goyal, Fan Zhang
/home/vagrant/dpdk/build/include/rte_crypto_sym.h:1009:4: warning: Value stored to 'left' is never read [deadcode.DeadStores]
left = 0;
^ ~
1 warning generated.
Compilator sees that the variable 'left' is never read after
assignment a '0' value. To get rid of this warning message, use 'if'
condition to verify the 'left' value before RTE_ASSERT.
Signed-off-by: Kamil Godzwon <kamilx.godzwon@intel.com>
---
lib/cryptodev/rte_crypto_sym.h | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/lib/cryptodev/rte_crypto_sym.h b/lib/cryptodev/rte_crypto_sym.h
index b43174dbec..d7183a0b9e 100644
--- a/lib/cryptodev/rte_crypto_sym.h
+++ b/lib/cryptodev/rte_crypto_sym.h
@@ -1016,7 +1016,10 @@ rte_crypto_mbuf_to_vec(const struct rte_mbuf *mb, uint32_t ofs, uint32_t len,
left -= seglen;
}
- RTE_ASSERT(left == 0);
+ if (left != 0) {
+ RTE_ASSERT(false);
+ }
+
return i;
}
--
2.40.1
---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.
Spolka oswiadcza, ze posiada status duzego przedsiebiorcy w rozumieniu ustawy z dnia 8 marca 2013 r. o przeciwdzialaniu nadmiernym opoznieniom w transakcjach handlowych.
Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH v2] lib/cryptodev: fix assertion to remove GCC compilation warning
2023-05-22 18:49 [PATCH] lib/cryptodev: fix assertion to remove GCC compilation warning Kamil Godzwon
@ 2023-05-22 19:04 ` Kamil Godzwon
2023-05-22 21:25 ` Stephen Hemminger
0 siblings, 1 reply; 6+ messages in thread
From: Kamil Godzwon @ 2023-05-22 19:04 UTC (permalink / raw)
To: dev; +Cc: Akhil Goyal, Fan Zhang
/home/vagrant/dpdk/build/include/rte_crypto_sym.h:1009:4: \
warning: Value stored to 'left' is never read [deadcode.DeadStores]
left = 0;
^ ~
1 warning generated.
Compilator sees that the variable 'left' is never read after
assignment a '0' value. To get rid of this warning message, use 'if'
condition to verify the 'left' value before RTE_ASSERT.
Signed-off-by: Kamil Godzwon <kamilx.godzwon@intel.com>
---
v2:
Changed commit message as the line was too long
Removed braces
---
lib/cryptodev/rte_crypto_sym.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/lib/cryptodev/rte_crypto_sym.h b/lib/cryptodev/rte_crypto_sym.h
index b43174dbec..dcef1a5049 100644
--- a/lib/cryptodev/rte_crypto_sym.h
+++ b/lib/cryptodev/rte_crypto_sym.h
@@ -1016,7 +1016,9 @@ rte_crypto_mbuf_to_vec(const struct rte_mbuf *mb, uint32_t ofs, uint32_t len,
left -= seglen;
}
- RTE_ASSERT(left == 0);
+ if (left != 0)
+ RTE_ASSERT(false);
+
return i;
}
--
2.40.1
---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.
Spolka oswiadcza, ze posiada status duzego przedsiebiorcy w rozumieniu ustawy z dnia 8 marca 2013 r. o przeciwdzialaniu nadmiernym opoznieniom w transakcjach handlowych.
Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] lib/cryptodev: fix assertion to remove GCC compilation warning
2023-05-22 19:04 ` [PATCH v2] " Kamil Godzwon
@ 2023-05-22 21:25 ` Stephen Hemminger
2023-05-23 8:12 ` [EXT] " Akhil Goyal
0 siblings, 1 reply; 6+ messages in thread
From: Stephen Hemminger @ 2023-05-22 21:25 UTC (permalink / raw)
To: Kamil Godzwon; +Cc: dev, Akhil Goyal, Fan Zhang
On Mon, 22 May 2023 15:04:52 -0400
Kamil Godzwon <kamilx.godzwon@intel.com> wrote:
> /home/vagrant/dpdk/build/include/rte_crypto_sym.h:1009:4: \
> warning: Value stored to 'left' is never read [deadcode.DeadStores]
> left = 0;
> ^ ~
> 1 warning generated.
>
> Compilator sees that the variable 'left' is never read after
> assignment a '0' value. To get rid of this warning message, use 'if'
> condition to verify the 'left' value before RTE_ASSERT.
>
> Signed-off-by: Kamil Godzwon <kamilx.godzwon@intel.com>
> ---
> v2:
> Changed commit message as the line was too long
> Removed braces
> ---
> lib/cryptodev/rte_crypto_sym.h | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/lib/cryptodev/rte_crypto_sym.h b/lib/cryptodev/rte_crypto_sym.h
> index b43174dbec..dcef1a5049 100644
> --- a/lib/cryptodev/rte_crypto_sym.h
> +++ b/lib/cryptodev/rte_crypto_sym.h
> @@ -1016,7 +1016,9 @@ rte_crypto_mbuf_to_vec(const struct rte_mbuf *mb, uint32_t ofs, uint32_t len,
> left -= seglen;
> }
>
> - RTE_ASSERT(left == 0);
> + if (left != 0)
> + RTE_ASSERT(false);
> +
> return i;
> }
>
This could happen if the passed in length to this routine was larger than
the amount of data in the mbuf. Should the function check and return an error?
Panic should only be reserved for seriously corrupted input (like invalid mbuf).
Also, this is a big enough function that it really should not be inlined.
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [EXT] Re: [PATCH v2] lib/cryptodev: fix assertion to remove GCC compilation warning
2023-05-22 21:25 ` Stephen Hemminger
@ 2023-05-23 8:12 ` Akhil Goyal
2023-05-23 16:00 ` Stephen Hemminger
0 siblings, 1 reply; 6+ messages in thread
From: Akhil Goyal @ 2023-05-23 8:12 UTC (permalink / raw)
To: Stephen Hemminger, Kamil Godzwon; +Cc: dev, Fan Zhang
> On Mon, 22 May 2023 15:04:52 -0400
> Kamil Godzwon <kamilx.godzwon@intel.com> wrote:
>
> > /home/vagrant/dpdk/build/include/rte_crypto_sym.h:1009:4: \
> > warning: Value stored to 'left' is never read [deadcode.DeadStores]
> > left = 0;
> > ^ ~
> > 1 warning generated.
> >
> > Compilator sees that the variable 'left' is never read after
> > assignment a '0' value. To get rid of this warning message, use 'if'
> > condition to verify the 'left' value before RTE_ASSERT.
> >
> > Signed-off-by: Kamil Godzwon <kamilx.godzwon@intel.com>
> > ---
> > v2:
> > Changed commit message as the line was too long
> > Removed braces
> > ---
> > lib/cryptodev/rte_crypto_sym.h | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/cryptodev/rte_crypto_sym.h b/lib/cryptodev/rte_crypto_sym.h
> > index b43174dbec..dcef1a5049 100644
> > --- a/lib/cryptodev/rte_crypto_sym.h
> > +++ b/lib/cryptodev/rte_crypto_sym.h
> > @@ -1016,7 +1016,9 @@ rte_crypto_mbuf_to_vec(const struct rte_mbuf
> *mb, uint32_t ofs, uint32_t len,
> > left -= seglen;
> > }
> >
> > - RTE_ASSERT(left == 0);
> > + if (left != 0)
> > + RTE_ASSERT(false);
> > +
> > return i;
> > }
> >
>
> This could happen if the passed in length to this routine was larger than
> the amount of data in the mbuf. Should the function check and return an error?
>
> Panic should only be reserved for seriously corrupted input (like invalid mbuf).
>
> Also, this is a big enough function that it really should not be inlined.
This is a datapath API. RTE_ASSERT is normally not enabled in release build.
So, this assert is not doing any check for normal scenario.
We normally avoid these type of error checks in the datapath.
And while building in debug mode, we need these asserts to give a backtrace also
To debug the rootcause of the issue.
I would suggest fixing the assert itself instead of adding a check.
Current patch will affect performance.
Agreed, that the function is big for being an inline function,
but that is what all the datapath APIs are and
we keep them inline to improve the performance.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [EXT] Re: [PATCH v2] lib/cryptodev: fix assertion to remove GCC compilation warning
2023-05-23 8:12 ` [EXT] " Akhil Goyal
@ 2023-05-23 16:00 ` Stephen Hemminger
2023-05-24 7:21 ` Akhil Goyal
0 siblings, 1 reply; 6+ messages in thread
From: Stephen Hemminger @ 2023-05-23 16:00 UTC (permalink / raw)
To: Akhil Goyal; +Cc: Kamil Godzwon, dev, Fan Zhang
On Tue, 23 May 2023 08:12:28 +0000
Akhil Goyal <gakhil@marvell.com> wrote:
> >
> > This could happen if the passed in length to this routine was larger than
> > the amount of data in the mbuf. Should the function check and return an error?
> >
> > Panic should only be reserved for seriously corrupted input (like invalid mbuf).
> >
> > Also, this is a big enough function that it really should not be inlined.
>
> This is a datapath API. RTE_ASSERT is normally not enabled in release build.
> So, this assert is not doing any check for normal scenario.
> We normally avoid these type of error checks in the datapath.
> And while building in debug mode, we need these asserts to give a backtrace also
> To debug the rootcause of the issue.
>
> I would suggest fixing the assert itself instead of adding a check.
> Current patch will affect performance.
>
> Agreed, that the function is big for being an inline function,
> but that is what all the datapath APIs are and
> we keep them inline to improve the performance.
Inline is not a magic go fast switch. Turns out that the compilers and cpu's
already do good job with functions. Using LTO helps too.
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [EXT] Re: [PATCH v2] lib/cryptodev: fix assertion to remove GCC compilation warning
2023-05-23 16:00 ` Stephen Hemminger
@ 2023-05-24 7:21 ` Akhil Goyal
0 siblings, 0 replies; 6+ messages in thread
From: Akhil Goyal @ 2023-05-24 7:21 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: Kamil Godzwon, dev, Fan Zhang
> On Tue, 23 May 2023 08:12:28 +0000
> Akhil Goyal <gakhil@marvell.com> wrote:
>
> > >
> > > This could happen if the passed in length to this routine was larger than
> > > the amount of data in the mbuf. Should the function check and return an
> error?
> > >
> > > Panic should only be reserved for seriously corrupted input (like invalid
> mbuf).
> > >
> > > Also, this is a big enough function that it really should not be inlined.
> >
> > This is a datapath API. RTE_ASSERT is normally not enabled in release build.
> > So, this assert is not doing any check for normal scenario.
> > We normally avoid these type of error checks in the datapath.
> > And while building in debug mode, we need these asserts to give a backtrace
> also
> > To debug the rootcause of the issue.
> >
> > I would suggest fixing the assert itself instead of adding a check.
> > Current patch will affect performance.
> >
> > Agreed, that the function is big for being an inline function,
> > but that is what all the datapath APIs are and
> > we keep them inline to improve the performance.
>
> Inline is not a magic go fast switch. Turns out that the compilers and cpu's
> already do good job with functions. Using LTO helps too.
Inline perform better in cases of small functions.
For big functions like the above one, it may not be much beneficial,
but it won't cause any harm as well.
Yes, LTO can be explored. Thanks for the suggestion.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-05-24 7:21 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-22 18:49 [PATCH] lib/cryptodev: fix assertion to remove GCC compilation warning Kamil Godzwon
2023-05-22 19:04 ` [PATCH v2] " Kamil Godzwon
2023-05-22 21:25 ` Stephen Hemminger
2023-05-23 8:12 ` [EXT] " Akhil Goyal
2023-05-23 16:00 ` Stephen Hemminger
2023-05-24 7:21 ` Akhil Goyal
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).