From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bruce Richardson Subject: Re: Question about store_return() in ~/dpdk/lib/librte_distributor/rte_distributor.c Date: Wed, 7 Oct 2015 12:04:14 +0100 Message-ID: <20151007110414.GA21348@bricha3-MOBL3> References: <47e2b06de73965fb579acd9170ae1f@cweb01.nm.nhnsystem.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Cc: dev@dpdk.org To: =?utf-8?B?7LWc7J217ISx?= Return-path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 418165A64 for ; Wed, 7 Oct 2015 13:04:18 +0200 (CEST) Content-Disposition: inline In-Reply-To: <47e2b06de73965fb579acd9170ae1f@cweb01.nm.nhnsystem.com> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Wed, Oct 07, 2015 at 06:07:23PM +0900, =EC=B5=9C=EC=9D=B5=EC=84=B1 wro= te: > Dear DPDK experts. > =20 > Thank you very much for your excellent efforts and contributions. > =20 > I have a question about store_return() in ~/dpdk/lib/librte_distributor= /rte_distributor.c > =20 > The store_return() function adds a oldbuf packet to d->returns.mbuf[= ] queue. > =20 > If the queue is full and the oldbuf packet is NULL, the queue seems to = lost a packet in the queue without modifying ret_start/ret_count values. > =20 > Is the last position of the queue always empty? > =20 > Would you check it?=20 > =20 > =20 > #define RTE_DISTRIB_MAX_RETURNS 128 > #define RTE_DISTRIB_RETURNS_MASK (RTE_DISTRIB_MAX_RETURNS - 1) > =20 > /* stores a packet returned from a worker inside the returns array */ > static inline void > store_return(uintptr_t oldbuf, struct rte_distributor *d, > unsigned *ret_start, unsigned *ret_count) > { > /* store returns in a circular buffer - code is branch-free. bu= ffer =EB=81=9D=EC=97=90 oldbuf=EB=A5=BC =EC=B6=94=EA=B0=80=ED=95=A8 */ > d->returns.mbufs[(*ret_start + *ret_count) & RTE_DISTRIB= _RETURNS_MASK] > =3D (void *)oldbuf; > *ret_start +=3D (*ret_count =3D=3D RTE_DISTRIB_RETURNS_MASK) &a= mp; !!(oldbuf); > *ret_count +=3D (*ret_count !=3D RTE_DISTRIB_RETURNS_MASK) &= ; !!(oldbuf); > } > =20 > If d->returns.mbufs[] queue is full, oldbuf replaces the first cell = of the queue (new packet overwrites the last packet in the queue). > =20 > ret_start is preserved if the queue is not full (count!=3D MAX_VALUE(RT= E_DISTRIB_RETURNS_MASK)) > if ret_start is MAX value and oldbuf is not NULL, ret_start is incremen= ted by 1. > =20 > ret_count is incremented by 1 if it is not MAX value and oldbuf is not = NULL). > if ret_count is MAX value, ret_count is preserved. > =20 > The mbufs queue is written by oldbuf(NULL) even though when the queue i= s full and the oldbuf packet is NULL (no packet insertion).=20 > It may lost a cell in the queue. > If the last position of the queue is always empty, it may not be a prob= lem. > =20 > Would you check it? > =20 > Thank you very much. > =20 > Sincerely Yours, > =20 > Ick-Sung Choi. >=20 Hi, yes, it can overwrite entries in that array. When designing the library, there were two possible use cases taken into = account: 1. The user did not want to return the packets to the distributor but pas= s them elsewhere from the workers when done. In this case, the overwriting did n= ot matter. 2. The second case is where ordering is to be preserved, and here the use= r does want to pass them back to the distributor. To accomodate this, the buffer= size is made considerably bigger than the default burst size, so that anyone w= ho polls the ring after each burst (or even after each second burst) should = again have no issues and avoid problems with overwriting the entry. Regards, /Bruce