From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124])
	by inbox.dpdk.org (Postfix) with ESMTP id A979CA0540;
	Mon,  4 Jul 2022 16:48:18 +0200 (CEST)
Received: from [217.70.189.124] (localhost [127.0.0.1])
	by mails.dpdk.org (Postfix) with ESMTP id 9782F42826;
	Mon,  4 Jul 2022 16:48:18 +0200 (CEST)
Received: from wout1-smtp.messagingengine.com (wout1-smtp.messagingengine.com
 [64.147.123.24]) by mails.dpdk.org (Postfix) with ESMTP id B23BB427F0
 for <dev@dpdk.org>; Mon,  4 Jul 2022 16:48:16 +0200 (CEST)
Received: from compute4.internal (compute4.nyi.internal [10.202.2.44])
 by mailout.west.internal (Postfix) with ESMTP id 1BD1F32005BC;
 Mon,  4 Jul 2022 10:48:15 -0400 (EDT)
Received: from mailfrontend2 ([10.202.2.163])
 by compute4.internal (MEProxy); Mon, 04 Jul 2022 10:48:15 -0400
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h=
 cc:cc:content-transfer-encoding:content-type:date:date:from:from
 :in-reply-to:in-reply-to:message-id:mime-version:references
 :reply-to:sender:subject:subject:to:to; s=fm2; t=1656946094; x=
 1657032494; bh=h+H6ePj1bkG3R36NYF/icMCWpryxAS+RIt3mxTaA7yU=; b=q
 MkI1DfCIzGOnlu6KJXCHrPL8SH+NjhvI8Vv57bmEMJjmkcr7e1CdTk/If8+U1x+F
 FYIGB7h3Fz8D9iMwLQrl5A12wyHQKTyU41urhattGrRZ0ZpDW11D9aaEazh3nZOr
 tUW2LlWXk0n6Ppq5WrYU3DSnBj5sFFm+NPMN4GjT4OFVrHam7rcdB8hAyhrh3hsD
 97Ml3WtMeOXd2gF/4uZHfvaImMB6NdtFI2Ccdw+/9H2p7ghvE7XkDZ1U+4dtrupE
 ThgIj3pP+UHtfeaobTgf2ltInbR+HFqxp0cJuoJJVX5EPC3W9zBhyC5CxUNRKPi7
 tdNiodJ4zmJ01aBsTbiPQ==
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=
 messagingengine.com; h=cc:cc:content-transfer-encoding
 :content-type:date:date:feedback-id:feedback-id:from:from
 :in-reply-to:in-reply-to:message-id:mime-version:references
 :reply-to:sender:subject:subject:to:to:x-me-proxy:x-me-proxy
 :x-me-sender:x-me-sender:x-sasl-enc; s=fm2; t=1656946094; x=
 1657032494; bh=h+H6ePj1bkG3R36NYF/icMCWpryxAS+RIt3mxTaA7yU=; b=F
 GnDaCc9VhHdkmr9wgbSnU/TbGc5EnhJ96Yrn4cfgYYpUqjOt9PhghEK0A3Svq80O
 nR2TgvGVV/KNRlMF6Top7iilIyJCyW6IpO7/XGIg2qDCJKuIk35TJJMcTWNehYD+
 JJNLlbYELjELvVuf0NPi21X0QYegk7QT4gDUczSDUmwiJL+4BLtMPhlm8n6RHQki
 nel6DBoskndHXsMYB0nRHhrUQ2/5/6gYVsQfpFCr9EfsmClatHnpRXOQMmCsdcxy
 Tg5Q+pYvoZoMNG60mrYf8tFZuLo70smDML/sn+GCi++A2iigDpoi7fESM8wVThzg
 /p7zB2RdGbd0SQr9YMlBw==
X-ME-Sender: <xms:rv3CYuAGZCYGXBVORUDfxprsP85oJc6VQ_5gaB7lcg0zs38wJ7Sm6Q>
 <xme:rv3CYohJOhVfA8Chv3WULsL1X1Maj4oOWZFQ9Cs5MPu1kEgSVjRkMjDX5oZilGxoN
 U5eM0AWxiLa_Avdug>
X-ME-Received: <xmr:rv3CYhnXoRgYZiuyzQdMsvqjAcfGIcx7s1KVXMnfxkw4l76whGz2rM5tm_GuP5mo64mB4x-BS2v4vslRJm6F66EPqw>
X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvfedrudehledgkedvucetufdoteggodetrfdotf
 fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen
 uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne
 cujfgurhephffvvefufffkjghfggfgtgesthfuredttddtvdenucfhrhhomhepvfhhohhm
 rghsucfoohhnjhgrlhhonhcuoehthhhomhgrshesmhhonhhjrghlohhnrdhnvghtqeenuc
 ggtffrrghtthgvrhhnpedtjeeiieefhedtfffgvdelteeufeefheeujefgueetfedttdei
 kefgkeduhedtgfenucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfh
 hrohhmpehthhhomhgrshesmhhonhhjrghlohhnrdhnvght
X-ME-Proxy: <xmx:rv3CYsx3GCLErUZjzuorR0ecrBnZbDx9KmdXv_g5ZWsT9CQ2X3Gz2A>
 <xmx:rv3CYjQOYBQvu-3lK1mW-6_rW8grRZ5ozx02f875VXpfddish3j6gg>
 <xmx:rv3CYnZgf6rXP0BdY3zmOFiBYRbbdcvnhT0x07V0Wn6tIx-jD7685g>
 <xmx:rv3CYiHkYFDEqntVu9b0_4AvcqgybS_XyAFgDAI2h1dmK4aXcjlIYA>
Feedback-ID: i47234305:Fastmail
Received: by mail.messagingengine.com (Postfix) with ESMTPA; Mon,
 4 Jul 2022 10:48:13 -0400 (EDT)
From: Thomas Monjalon <thomas@monjalon.net>
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>, jerinj@marvell.com,
 gakhil@marvell.com
Subject: Re: [PATCH v3 1/2] examples/l3fwd: common packet group functionality
Date: Mon, 04 Jul 2022 16:48:12 +0200
Message-ID: <3162621.Bm8zEkEi59@thomas>
In-Reply-To: <20220623093816.254830-1-rbhansali@marvell.com>
References: <20220524095717.3875284-1-rbhansali@marvell.com>
 <20220623093816.254830-1-rbhansali@marvell.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org

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.