From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <thomas@monjalon.net>
Received: from out1-smtp.messagingengine.com (out1-smtp.messagingengine.com
 [66.111.4.25]) by dpdk.org (Postfix) with ESMTP id 6B0241BEE5
 for <dev@dpdk.org>; Wed,  4 Jul 2018 14:27:04 +0200 (CEST)
Received: from compute1.internal (compute1.nyi.internal [10.202.2.41])
 by mailout.nyi.internal (Postfix) with ESMTP id EAF9721903;
 Wed,  4 Jul 2018 08:27:03 -0400 (EDT)
Received: from mailfrontend2 ([10.202.2.163])
 by compute1.internal (MEProxy); Wed, 04 Jul 2018 08:27:03 -0400
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h=
 cc:content-transfer-encoding:content-type:date:from:in-reply-to
 :message-id:mime-version:references:subject:to:x-me-sender
 :x-me-sender:x-sasl-enc; s=mesmtp; bh=kOCSvHGln53eo7nZz8ZnI/aB7E
 X5lrMx/JBuSTqhOCM=; b=JPTmLNwgG98CYg+IW2Y2IZTl1JgX1Kj34B8uoTMBQI
 cFIoHpyH2/bgDd6iTDTigBoiDKgICgzKetBycbRmtw9QMvfy2BjS7hPrGnN6JTWY
 wYa+qfsrbe53FV58tnDWmx7FBTBygx3rufuowNp4EywuCsJJgEI95cD5wF1ONXME
 w=
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=
 messagingengine.com; h=cc:content-transfer-encoding:content-type
 :date:from:in-reply-to:message-id:mime-version:references
 :subject:to:x-me-sender:x-me-sender:x-sasl-enc; s=fm3; bh=kOCSvH
 Gln53eo7nZz8ZnI/aB7EX5lrMx/JBuSTqhOCM=; b=UucbANCazBfcfpde21LSyD
 EOrCfMdAd/Jh1QsEEHBB7QCTL6IfECNFECr//hqWKb8bKvM8uqzxK0USRgYFNqUJ
 HB4ziB5ANmiMqGlgozpIbrWBRrE1CwI51MHwHsYNIHSCGiFvi0nJ/uZESz6cu+9P
 Jkh1QKTjII0Ttcb0jWu2cUSJKVEyfhfqcWUpqojiO89LyNPWaUmDQakhvLm4c9Ji
 EWxkKgFMy/bzcV3W3otS2Idiv1Gy9le8Mmp76t8MNCkqFItsSsHFuoYpj2u3wATh
 cyb8f8WGn/kdavrTeWZX7yRosSFRvhgX3UEK4yuhzDRluhLuTpx5N78AB/gCo1wQ
 ==
X-ME-Proxy: <xmx:Fr08W_OKU1I3dMrXEsIQgKnJP8fDfGMBi13lBrk3NR1nGkSpTW9SUw>
 <xmx:Fr08W3k2DDyV2C2FJz81SWQHbWRBhjP9UiEmbJrArCnctKREXKbkaA>
 <xmx:Fr08W1p56H78QNpTKCSFvfmYRgJkzkCjRVg0g3ihVdn-yGVBakpX6w>
 <xmx:Fr08W75TIqPPhvprR9-2wjpjfG6EsaUqt37N1qW3dmxL81IqwCkRBg>
 <xmx:Fr08W_IT6k1PgG2XcRzngN9o3VLGchiVL1-tyyPHDB4b0vQPrkcJag>
 <xmx:F708W9z7qYlfwOzROFwTtk4Vsh5rjxDNO_8-sV1dGCEKFAofyly1mg>
X-ME-Sender: <xms:Fr08WwdSySqHvh5zHHRB8GX-bQZhPjrh3cpgUUfuGkm0pR9thOESeQ>
Received: from xps.localnet (184.203.134.77.rev.sfr.net [77.134.203.184])
 by mail.messagingengine.com (Postfix) with ESMTPA id 64184102CF;
 Wed,  4 Jul 2018 08:27:01 -0400 (EDT)
From: Thomas Monjalon <thomas@monjalon.net>
To: Andrew Rybchenko <arybchenko@solarflare.com>, ferruh.yigit@intel.com
Cc: shahafs@mellanox.com, dev@dpdk.org, ravi1.kumar@amd.com,
 rasesh.mody@cavium.com, maxime.coquelin@redhat.com
Date: Wed, 04 Jul 2018 14:26:58 +0200
Message-ID: <2074528.iGUK7qq6Nh@xps>
In-Reply-To: <1d842244-297a-1e0d-c86c-989a99b6c3c8@solarflare.com>
References: <1571938.317irMz1sZ@xps>
 <20180702212750.16758-4-thomas@monjalon.net>
 <1d842244-297a-1e0d-c86c-989a99b6c3c8@solarflare.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Subject: Re: [dpdk-dev] [PATCH v2 3/5] ethdev: convert remaining apps to new
	offload API
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
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>
X-List-Received-Date: Wed, 04 Jul 2018 12:27:04 -0000

04/07/2018 13:16, Andrew Rybchenko:
> On 07/03/2018 12:27 AM, Thomas Monjalon wrote:
> > --- a/doc/guides/sample_app_ug/link_status_intr.rst
> > +++ b/doc/guides/sample_app_ug/link_status_intr.rst
> > @@ -137,10 +137,7 @@ The global configuration is stored in a static structure:
> >       static const struct rte_eth_conf port_conf = {
> >           .rxmode = {
> >               .split_hdr_size = 0,
> > -            .header_split = 0,   /**< Header Split disabled */
> > -            .hw_ip_checksum = 0, /**< IP checksum offload disabled */
> > -            .hw_vlan_filter = 0, /**< VLAN filtering disabled */
> > -            .hw_strip_crc= 0,    /**< CRC stripped by hardware */
> > +            .offloads = DEV_RX_OFFLOAD_CRC_STRIP,
> 
> Is it intended that CRC strip was disabled before and now it is becoming 
> enabled?

Yes, I consider the comment to be the real intent.

> > --- a/examples/bbdev_app/main.c
> > +++ b/examples/bbdev_app/main.c
> > @@ -64,11 +64,7 @@ static const struct rte_eth_conf port_conf = {
> >   		.mq_mode = ETH_MQ_RX_NONE,
> >   		.max_rx_pkt_len = ETHER_MAX_LEN,
> >   		.split_hdr_size = 0,
> > -		.header_split = 0, /**< Header Split disabled */
> > -		.hw_ip_checksum = 0, /**< IP checksum offload disabled */
> > -		.hw_vlan_filter = 0, /**< VLAN filtering disabled */
> > -		.jumbo_frame = 0, /**< Jumbo Frame Support disabled */
> > -		.hw_strip_crc = 0, /**< CRC stripped by hardware */
> > +		.offloads = DEV_RX_OFFLOAD_CRC_STRIP,
> 
> Is it intended that CRC strip was disabled before and now it is becoming 
> enabled?

Yes, I consider the comment to be the real intent.

> > --- a/test/test/test_pmd_perf.c
> > +++ b/test/test/test_pmd_perf.c
> > @@ -97,11 +90,6 @@ static struct rte_eth_txconf tx_conf = {
> >   	},
> >   	.tx_free_thresh = 32, /* Use PMD default values */
> >   	.tx_rs_thresh = 32, /* Use PMD default values */
> > -	.txq_flags = (ETH_TXQ_FLAGS_NOMULTSEGS |
> > -		      ETH_TXQ_FLAGS_NOVLANOFFL |
> > -		      ETH_TXQ_FLAGS_NOXSUMSCTP |
> > -		      ETH_TXQ_FLAGS_NOXSUMUDP |
> > -		      ETH_TXQ_FLAGS_NOXSUMTCP)
> >   };
> >   
> >   enum {
> > @@ -808,38 +796,29 @@ test_set_rxtx_conf(cmdline_fixed_string_t mode)
> >   
> >   	if (!strcmp(mode, "vector")) {
> >   		/* vector rx, tx */
> > -		tx_conf.txq_flags = 0xf01;
> 
> I'd say that 100% correct equivalent would be:
> tx_conf.offloads &= ~(DEV_TX_OFFLOAD_VLAN_INSERT |
>            DEV_TX_OFFLOAD_IPV4_CKSUM | DEV_TX_OFFLOAD_UDP_CKSUM |
>            DEV_TX_OFFLOAD_TCP_CKSUM | DEV_TX_OFFLOAD_SCTP_CKSUM |
>            DEV_TX_OFFLOAD_MULTI_SEGS);

I'd say it is a really crappy code, and probably tuned for Intel devices only.

> 
> I guess the function may be called few times with different mode set.
> If so, similar fixes should be applied below as well.
> 
> >   		tx_conf.tx_rs_thresh = 32;
> >   		tx_conf.tx_free_thresh = 32;
> > -		port_conf.rxmode.hw_ip_checksum = 0;
> > -		port_conf.rxmode.enable_scatter = 0;
> 
> port_conf.rxmode.offloads &= ~(DEV_RX_OFFLOAD_CHECKSUM | 
> DEV_RX_OFFLOAD_SCATTER);
> 
> >   		return 0;
> >   	} else if (!strcmp(mode, "scalar")) {
> >   		/* bulk alloc rx, full-featured tx */
> > -		tx_conf.txq_flags = 0;
> 
> I think here we should enable offloads listed above to have 
> full-featured Tx:
> tx_conf.offloads |=  ...
> 
> >   		tx_conf.tx_rs_thresh = 32;
> >   		tx_conf.tx_free_thresh = 32;
> > -		port_conf.rxmode.hw_ip_checksum = 1;
> > -		port_conf.rxmode.enable_scatter = 0;
> > +		port_conf.rxmode.offloads |= DEV_RX_OFFLOAD_CHECKSUM;
> 
> port_conf.rxmode.offloads &= ~DEV_RX_OFFLOAD_SCATTER;
> 
> >   		return 0;
> >   	} else if (!strcmp(mode, "hybrid")) {
> >   		/* bulk alloc rx, vector tx
> >   		 * when vec macro not define,
> >   		 * using the same rx/tx as scalar
> >   		 */
> > -		tx_conf.txq_flags = 0xf01;
> 
> As in similar case above.
> 
> >   		tx_conf.tx_rs_thresh = 32;
> >   		tx_conf.tx_free_thresh = 32;
> > -		port_conf.rxmode.hw_ip_checksum = 1;
> > -		port_conf.rxmode.enable_scatter = 0;
> > +		port_conf.rxmode.offloads |= DEV_RX_OFFLOAD_CHECKSUM;
> 
> As in similar case above
> 
> >   		return 0;
> >   	} else if (!strcmp(mode, "full")) {
> >   		/* full feature rx,tx pair */
> > -		tx_conf.txq_flags = 0x0;   /* must condition */
> 
> As in similar case above.
> 
> >   		tx_conf.tx_rs_thresh = 32;
> >   		tx_conf.tx_free_thresh = 32;
> > -		port_conf.rxmode.hw_ip_checksum = 0;
> > -		port_conf.rxmode.enable_scatter = 1; /* must condition */
> > +		port_conf.rxmode.offloads |= DEV_RX_OFFLOAD_SCATTER;
> 
> port_conf.rxmode.offloads &= ~DEV_RX_OFFLOAD_CHECKSUM;
> 
> >   		return 0;
> >   	}
> >   
> 
> In general I think that it would be really good to avoid changes in
> behaviour when technical changes are done.

I agree, but in this case, it is impossible to know what was the real intent.
And I am perfectly fine breaking bad code.
The other option is to just remove the file. Maybe the best option?