> -----Original Message-----
> From: Thomas Monjalon <tho...@monjalon.net>
> Sent: Monday, July 4, 2022 8:18 PM
> To: Rahul Bhansali <rbhans...@marvell.com>
> Cc: dev@dpdk.org; David Christensen <d...@linux.vnet.ibm.com>; Ruifeng Wang
> <ruifeng.w...@arm.com>; Bruce Richardson <bruce.richard...@intel.com>;
> Konstantin Ananyev <konstantin.v.anan...@yandex.ru>; Jerin Jacob
> Kollanukkaran <jer...@marvell.com>; Akhil Goyal <gak...@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.
>
