From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by dpdk.org (Postfix) with ESMTP id 287C4101B for ; Wed, 20 Sep 2017 15:21:38 +0200 (CEST) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga105.jf.intel.com with ESMTP; 20 Sep 2017 06:21:37 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,421,1500966000"; d="scan'208";a="902207204" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.237.220.57]) ([10.237.220.57]) by FMSMGA003.fm.intel.com with ESMTP; 20 Sep 2017 06:21:36 -0700 To: Chas Williams <3chas3@gmail.com>, dev@dpdk.org Cc: linville@tuxdriver.com, "Charles (Chas) Williams" , Chas Williams References: <20170919212131.7006-1-3chas3@gmail.com> <20170919214545.7549-1-3chas3@gmail.com> From: Ferruh Yigit Message-ID: <0b7b3864-7594-5737-8f23-8dd8420a1c54@intel.com> Date: Wed, 20 Sep 2017 14:21:35 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <20170919214545.7549-1-3chas3@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v2] net/af_packet: make bypass configurable 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: , X-List-Received-Date: Wed, 20 Sep 2017 13:21:39 -0000 On 9/19/2017 10:45 PM, Chas Williams wrote: > From: "Charles (Chas) Williams" > > In certain situations, low speed interfaces, it may be desirable to > have the flow control provided by the kernel queueing disciplines. Out of curiosity, do you have any compression of performance numbers with and without qdisc? > > Signed-off-by: Chas Williams > --- > drivers/net/af_packet/rte_eth_af_packet.c | 22 +++++++++++++++++----- > 1 file changed, 17 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/af_packet/rte_eth_af_packet.c b/drivers/net/af_packet/rte_eth_af_packet.c > index 7aa26e5..e3858fa 100644 > --- a/drivers/net/af_packet/rte_eth_af_packet.c > +++ b/drivers/net/af_packet/rte_eth_af_packet.c > @@ -59,6 +59,7 @@ > #define ETH_AF_PACKET_BLOCKSIZE_ARG "blocksz" > #define ETH_AF_PACKET_FRAMESIZE_ARG "framesz" > #define ETH_AF_PACKET_FRAMECOUNT_ARG "framecnt" > +#define ETH_AF_PACKET_BYPASS_ARG "bypass" I thinks argument name "bypass" on its own is not clear what is bypassed, would you mind using something like "qdisc_bypass"? > > #define DFLT_BLOCK_SIZE (1 << 12) > #define DFLT_FRAME_SIZE (1 << 11) > @@ -115,6 +116,7 @@ static const char *valid_arguments[] = { > ETH_AF_PACKET_BLOCKSIZE_ARG, > ETH_AF_PACKET_FRAMESIZE_ARG, > ETH_AF_PACKET_FRAMECOUNT_ARG, > + ETH_AF_PACKET_BYPASS_ARG, Same comment here, what about "ETH_AF_PACKET_QDISC_BYPASS_ARG" ? > NULL > }; > > @@ -559,6 +561,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev, > unsigned int blockcnt, > unsigned int framesize, > unsigned int framecnt, > + unsigned int bypass, > struct pmd_internals **internals, > struct rte_eth_dev **eth_dev, > struct rte_kvargs *kvlist) > @@ -580,9 +583,6 @@ rte_pmd_init_internals(struct rte_vdev_device *dev, > #if defined(PACKET_FANOUT) > int fanout_arg; > #endif > -#if defined(PACKET_QDISC_BYPASS) > - int bypass; > -#endif > > for (k_idx = 0; k_idx < kvlist->count; k_idx++) { > pair = &kvlist->pairs[k_idx]; > @@ -698,7 +698,6 @@ rte_pmd_init_internals(struct rte_vdev_device *dev, > } > > #if defined(PACKET_QDISC_BYPASS) > - bypass = 1; > rc = setsockopt(qsockfd, SOL_PACKET, PACKET_QDISC_BYPASS, > &bypass, sizeof(bypass)); > if (rc == -1) { > @@ -851,6 +850,7 @@ rte_eth_from_packet(struct rte_vdev_device *dev, > unsigned int framesize = DFLT_FRAME_SIZE; > unsigned int framecount = DFLT_FRAME_COUNT; > unsigned int qpairs = 1; > + unsigned int bypass = 1; > > /* do some parameter checking */ > if (*sockfd < 0) > @@ -902,6 +902,16 @@ rte_eth_from_packet(struct rte_vdev_device *dev, > } > continue; > } > + if (strstr(pair->key, ETH_AF_PACKET_BYPASS_ARG) != NULL) { > + bypass = atoi(pair->value); > + if (bypass > 2) { This accepts "2" too . As far as I can see kernel side checks if this value is zero or not, so perhaps this check is not required at all. But if you want to keep it I guess intention is to limit the value to "0" and "1". > + RTE_LOG(ERR, PMD, > + "%s: invalid bypass value\n", > + name); > + return -1; > + } > + continue; > + } > } > > if (framesize > blocksize) { > @@ -927,6 +937,7 @@ rte_eth_from_packet(struct rte_vdev_device *dev, > if (rte_pmd_init_internals(dev, *sockfd, qpairs, > blocksize, blockcount, > framesize, framecount, > + bypass, > &internals, ð_dev, > kvlist) < 0) > return -1; > @@ -1021,4 +1032,5 @@ RTE_PMD_REGISTER_PARAM_STRING(net_af_packet, > "qpairs= " > "blocksz= " > "framesz= " > - "framecnt="); > + "framecnt= " > + "bypass=" Although storage is integer, does is make sense to document it as "0|1" to clarify the usage? ); >