From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by dpdk.space (Postfix) with ESMTP id CDD20A0096 for ; Thu, 11 Apr 2019 04:30:23 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id D7F0C5587; Thu, 11 Apr 2019 04:30:21 +0200 (CEST) Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by dpdk.org (Postfix) with ESMTP id E92EF54AE for ; Thu, 11 Apr 2019 04:30:19 +0200 (CEST) X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 10 Apr 2019 19:30:18 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.60,335,1549958400"; d="scan'208";a="222432627" Received: from yexl-server.sh.intel.com (HELO localhost) ([10.67.110.206]) by orsmga001.jf.intel.com with ESMTP; 10 Apr 2019 19:30:16 -0700 Date: Thu, 11 Apr 2019 10:24:56 +0800 From: Ye Xiaolong To: David Marchand Cc: Karlsson Magnus , dev , Ferruh Yigit , Qi Zhang , Topel Bjorn Message-ID: <20190411022456.GA114383@intel.com> References: <20190409151902.14675-1-xiaolong.ye@intel.com> <20190410105327.110410-1-xiaolong.ye@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.9.4 (2018-02-28) Subject: Re: [dpdk-dev] [PATCH] net/af_xdp: submit valid count to Tx queue X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Message-ID: <20190411022456.WdCzE4semb8b1kvuSgLl2LLoV-fr9hHv-PKshMAr_O4@z> On 04/10, David Marchand wrote: >On Wed, Apr 10, 2019 at 12:58 PM Xiaolong Ye wrote: > >> Since tx callback only picks mbufs that can be copied to the xdp desc, it >> should call xsk_ring_prod__submit with valid count other than nb_pkts. >> >> Fixes: f1debd77efaf ("net/af_xdp: introduce AF_XDP PMD") >> >> Reported-by: David Marchand >> Signed-off-by: Xiaolong Ye >> --- >> drivers/net/af_xdp/rte_eth_af_xdp.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c >> b/drivers/net/af_xdp/rte_eth_af_xdp.c >> index 5cc643ce2..8c2ba5740 100644 >> --- a/drivers/net/af_xdp/rte_eth_af_xdp.c >> +++ b/drivers/net/af_xdp/rte_eth_af_xdp.c >> @@ -300,7 +300,7 @@ eth_af_xdp_tx(void *queue, struct rte_mbuf **bufs, >> uint16_t nb_pkts) >> rte_pktmbuf_free(mbuf); >> } >> >> - xsk_ring_prod__submit(&txq->tx, nb_pkts); >> + xsk_ring_prod__submit(&txq->tx, valid); >> > >Err, well, I think we will have an issue here. > >Taking from the 5.1.0-rc4 sources I have: > >static inline __u32 xsk_prod_nb_free(struct xsk_ring_prod *r, __u32 nb) >{ > __u32 free_entries = r->cached_cons - r->cached_prod; > > if (free_entries >= nb) > return free_entries; > > /* Refresh the local tail pointer. > * cached_cons is r->size bigger than the real consumer pointer so > * that this addition can be avoided in the more frequently > * executed code that computs free_entries in the beginning of > * this function. Without this optimization it whould have been > * free_entries = r->cached_prod - r->cached_cons + r->size. > */ > r->cached_cons = *r->consumer + r->size; > > return r->cached_cons - r->cached_prod; >} > >static inline size_t xsk_ring_prod__reserve(struct xsk_ring_prod *prod, > size_t nb, __u32 *idx) >{ > if (unlikely(xsk_prod_nb_free(prod, nb) < nb)) > return 0; > > *idx = prod->cached_prod; > prod->cached_prod += nb; > > return nb; >} > >static inline void xsk_ring_prod__submit(struct xsk_ring_prod *prod, size_t >nb) >{ > /* Make sure everything has been written to the ring before >signalling > * this to the kernel. > */ > smp_wmb(); > > *prod->producer += nb; >} > > >If we reserve N slots, but only submit n slots, we end up with an incorrect >opinion of the number of available slots later. >Either xsk_ring_prod__submit should also update cached_prod (but I am not >sure it was the intent of this api), or we must ensure that both reserve >and submit are consistent. I think you are right, current design does have the flaw, I haven't thought of it before :( So in order to make sure both reserve and submit are consistent, what about we check the valid count of mbuf at the beginning, then reserve the valid count slots? Thanks, Xiaolong > >Did I miss some subtility ? > > >-- >David Marchand