RE: [EXT] Re: [PATCH v3 1/2] examples/l3fwd: common packet group functionality

[Rahul Bhansali <rbhansali@marvell.com>]

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Monday, July 4, 2022 8:18 PM
> To: Rahul Bhansali <rbhansali@marvell.com>
> Cc: dev@dpdk.org; David Christensen <drc@linux.vnet.ibm.com>; Ruifeng Wang
> <ruifeng.wang@arm.com>; Bruce Richardson <bruce.richardson@intel.com>;
> Konstantin Ananyev <konstantin.v.ananyev@yandex.ru>; Jerin Jacob
> Kollanukkaran <jerinj@marvell.com>; Akhil Goyal <gakhil@marvell.com>
> Subject: [EXT] Re: [PATCH v3 1/2] examples/l3fwd: common packet group
> functionality
> 
> External Email
> 
> ----------------------------------------------------------------------
> 23/06/2022 11:38, Rahul Bhansali:
> > +#ifndef _PORT_GROUP_H_
> > +#define _PORT_GROUP_H_
> 
> No need of underscores at begin and end.
> 
> > +
> > +#include "pkt_group.h"
> > +
> > +/*
> > + * Group consecutive packets with the same destination port in bursts of 4.
> > + * Suppose we have array of destination ports:
> > + * dst_port[] = {a, b, c, d,, e, ... }
> > + * dp1 should contain: <a, b, c, d>, dp2: <b, c, d, e>.
> > + * We doing 4 comparisons at once and the result is 4 bit mask.
> > + * This mask is used as an index into prebuild array of pnum values.
> > + */
> 
> This explanation is not clear to me.
> 
> > +static inline uint16_t *
> > +port_groupx4(uint16_t pn[FWDSTEP + 1], uint16_t *lp,
> 
> array parameter is not standard, you should make it a simple pointer
> 
> > +	     __vector unsigned short dp1,
> > +	     __vector unsigned short dp2)
> 
> 
> longer parameter names would help
> 
> [...]
> > --- a/examples/l3fwd/Makefile
> > +++ b/examples/l3fwd/Makefile
> > +INCLUDES =-I../common
> >  PC_FILE := $(shell $(PKGCONF) --path libdpdk 2>/dev/null)  CFLAGS +=
> > -O3 $(shell $(PKGCONF) --cflags libdpdk)  # Added for
> > 'rte_eth_link_to_str()'
> > @@ -38,10 +39,10 @@ endif
> >  endif
> >
> >  build/$(APP)-shared: $(SRCS-y) Makefile $(PC_FILE) | build
> > -	$(CC) $(CFLAGS) $(SRCS-y) -o $@ $(LDFLAGS) $(LDFLAGS_SHARED)
> > +	$(CC) $(CFLAGS) $(SRCS-y) $(INCLUDES) -o $@ $(LDFLAGS)
> > +$(LDFLAGS_SHARED)
> >
> >  build/$(APP)-static: $(SRCS-y) Makefile $(PC_FILE) | build
> > -	$(CC) $(CFLAGS) $(SRCS-y) -o $@ $(LDFLAGS) $(LDFLAGS_STATIC)
> > +	$(CC) $(CFLAGS) $(SRCS-y) $(INCLUDES) -o $@ $(LDFLAGS)
> > +$(LDFLAGS_STATIC)
> 
> No need to introduce INCLUDES, you can expand CFLAGS.
> 
> I will fix this last one while pulling.
> Please work on better names and explanations for an EAL integration.
> 
Ack, will make changes for an EAL integration.

>