diff mbox series

[02/11] examples/l3fwd: fix scalar LPM compilation

Message ID 20220505173003.3242618-3-kda@semihalf.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series Introduce support for RISC-V architecture |

Checks

Context Check Description
ci/checkpatch warning coding style issues

Commit Message

Stanislaw Kardach May 5, 2022, 5:29 p.m. UTC 
  The lpm_process_event_pkt() can either process a packet using an
architecture specific (defined for X86/SSE, ARM/Neon and PPC64/Altivec)
path or a scalar one. The choice is however done using an ifdef
pre-processor macro. Because of that the scalar version was apparently
not widely excersized/compiled.
Due to some copy/paste errors, the scalar logic in
lpm_process_event_pkt() retained a "continue" statement where a BAD_PORT
should be returned after refactoring of the LPM logic in the l3fwd
example.

Fixes: 99fc91d18082 ("examples/l3fwd: add event lpm main loop")
Cc: pbhagavatula@marvell.com

Signed-off-by: Stanislaw Kardach <kda@semihalf.com>
Sponsored-by: Frank Zhao <Frank.Zhao@starfivetech.com>
Sponsored-by: Sam Grove <sam.grove@sifive.com>
---
 examples/l3fwd/l3fwd_lpm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Stephen Hemminger May 5, 2022, 5:39 p.m. UTC | #1 
On Thu,  5 May 2022 19:29:54 +0200
Stanislaw Kardach <kda@semihalf.com> wrote:

> The lpm_process_event_pkt() can either process a packet using an
> architecture specific (defined for X86/SSE, ARM/Neon and PPC64/Altivec)
> path or a scalar one. The choice is however done using an ifdef
> pre-processor macro. Because of that the scalar version was apparently
> not widely excersized/compiled.
> Due to some copy/paste errors, the scalar logic in
> lpm_process_event_pkt() retained a "continue" statement where a BAD_PORT
> should be returned after refactoring of the LPM logic in the l3fwd
> example.
> 
> Fixes: 99fc91d18082 ("examples/l3fwd: add event lpm main loop")
> Cc: pbhagavatula@marvell.com
> 
> Signed-off-by: Stanislaw Kardach <kda@semihalf.com>
> Sponsored-by: Frank Zhao <Frank.Zhao@starfivetech.com>
> Sponsored-by: Sam Grove <sam.grove@sifive.com>

Would be easier to get merged if bug fixes came as separate patch
submission.

Also have not seen Sponsored-by before; what do you expect it to mean?
Never used in DPDK or kernel git tree.
Stanislaw Kardach May 5, 2022, 5:49 p.m. UTC | #2 
On Thu, May 5, 2022 at 7:39 PM Stephen Hemminger <stephen@networkplumber.org>
wrote:

> On Thu,  5 May 2022 19:29:54 +0200
> Stanislaw Kardach <kda@semihalf.com> wrote:
>
> > The lpm_process_event_pkt() can either process a packet using an
> > architecture specific (defined for X86/SSE, ARM/Neon and PPC64/Altivec)
> > path or a scalar one. The choice is however done using an ifdef
> > pre-processor macro. Because of that the scalar version was apparently
> > not widely excersized/compiled.
> > Due to some copy/paste errors, the scalar logic in
> > lpm_process_event_pkt() retained a "continue" statement where a BAD_PORT
> > should be returned after refactoring of the LPM logic in the l3fwd
> > example.
> >
> > Fixes: 99fc91d18082 ("examples/l3fwd: add event lpm main loop")
> > Cc: pbhagavatula@marvell.com
> >
> > Signed-off-by: Stanislaw Kardach <kda@semihalf.com>
> > Sponsored-by: Frank Zhao <Frank.Zhao@starfivetech.com>
> > Sponsored-by: Sam Grove <sam.grove@sifive.com>
>
> Would be easier to get merged if bug fixes came as separate patch
> submission.
>
Sure, I can post this separately. The reason for posting this along with
RISC-V patches is that those depend on this one. So I could add
"depends-on" but wanted be on the safe side.

>
> Also have not seen Sponsored-by before; what do you expect it to mean?
> Never used in DPDK or kernel git tree.
>
The idea is that this work was sponsored by the companies mentioned in the
sign-off. It is used i.e. in FreeBSD though admittedly never in Linux or
DPDK.
Alternative, which makes checkpatch happy and was previously used is
"Suggested-by". However suggestion, doesn't necessary mean sponsorship.
I had a talk about this with Thomas Monjalon and he has also leaned towards
"Sponsored-by".
I'm open to suggestions as I admit, I'm not sure which route is better.
Stephen Hemminger May 5, 2022, 6:09 p.m. UTC | #3 
On Thu, 5 May 2022 19:49:28 +0200
Stanisław Kardach <kda@semihalf.com> wrote:

> On Thu, May 5, 2022 at 7:39 PM Stephen Hemminger <stephen@networkplumber.org>
> wrote:
> 
> > On Thu,  5 May 2022 19:29:54 +0200
> > Stanislaw Kardach <kda@semihalf.com> wrote:
> >  
> > > The lpm_process_event_pkt() can either process a packet using an
> > > architecture specific (defined for X86/SSE, ARM/Neon and PPC64/Altivec)
> > > path or a scalar one. The choice is however done using an ifdef
> > > pre-processor macro. Because of that the scalar version was apparently
> > > not widely excersized/compiled.
> > > Due to some copy/paste errors, the scalar logic in
> > > lpm_process_event_pkt() retained a "continue" statement where a BAD_PORT
> > > should be returned after refactoring of the LPM logic in the l3fwd
> > > example.
> > >
> > > Fixes: 99fc91d18082 ("examples/l3fwd: add event lpm main loop")
> > > Cc: pbhagavatula@marvell.com
> > >
> > > Signed-off-by: Stanislaw Kardach <kda@semihalf.com>
> > > Sponsored-by: Frank Zhao <Frank.Zhao@starfivetech.com>
> > > Sponsored-by: Sam Grove <sam.grove@sifive.com>  
> >
> > Would be easier to get merged if bug fixes came as separate patch
> > submission.
> >  
> Sure, I can post this separately. The reason for posting this along with
> RISC-V patches is that those depend on this one. So I could add
> "depends-on" but wanted be on the safe side.
> 
> >
> > Also have not seen Sponsored-by before; what do you expect it to mean?
> > Never used in DPDK or kernel git tree.
> >  
> The idea is that this work was sponsored by the companies mentioned in the
> sign-off. It is used i.e. in FreeBSD though admittedly never in Linux or
> DPDK.
> Alternative, which makes checkpatch happy and was previously used is
> "Suggested-by". However suggestion, doesn't necessary mean sponsorship.
> I had a talk about this with Thomas Monjalon and he has also leaned towards
> "Sponsored-by".
> I'm open to suggestions as I admit, I'm not sure which route is better.

So it is just advertising.

I did notice slightly different syntax in the kernel.
Could we follow that precedent?

Example:


commit 0301bcd599e552c38adf6771c25ff99680b9c4ee
Author: Bjoern A. Zeeb <bz@FreeBSD.ORG>
Date:   Fri Jan 28 15:34:26 2022 +0200

    iwlwifi: de-const properly where needed
    
    In order to de-const variables simply casting through (void *) is
    not enough: "cast from 'const .. *' to 'void *' drops const qualifier".
    Cast through (uintptr_t) as well [1] to make this compile on systems
    with more strict requirements.
    In addition passing const void *data to dma_map_single() also
    drops the (const) qualifier.  De-constify on variable on assignment
    which may be overwritten later.  In either case the (void *) cast
    to dma_map_single() is not needed (anymore) either.
    
    [1] See __DECONST() in sys/sys/cdefs.h in FreeBSD
    
    Sponsored by:  The FreeBSD Foundation
    Signed-off-by: Bjoern A. Zeeb <bz@FreeBSD.ORG>
    Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
    Link: https://lore.kernel.org/r/iwlwifi.20220128153014.eb696eb56bf6.Ide1dd041f9b908c5154a600286a7453750b0704a@changeid
    Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
diff mbox series

Patch

diff --git a/examples/l3fwd/l3fwd_lpm.c b/examples/l3fwd/l3fwd_lpm.c
index bec22c44cd..6e1defbf7f 100644
--- a/examples/l3fwd/l3fwd_lpm.c
+++ b/examples/l3fwd/l3fwd_lpm.c
@@ -248,7 +248,7 @@  lpm_process_event_pkt(const struct lcore_conf *lconf, struct rte_mbuf *mbuf)
 		if (is_valid_ipv4_pkt(ipv4_hdr, mbuf->pkt_len)
 				< 0) {
 			mbuf->port = BAD_PORT;
-			continue;
+			return mbuf->port;
 		}
 		/* Update time to live and header checksum */
 		--(ipv4_hdr->time_to_live);