From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from out1-smtp.messagingengine.com (out1-smtp.messagingengine.com [66.111.4.25]) by dpdk.org (Postfix) with ESMTP id 6B0241BEE5 for ; 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: X-ME-Sender: 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 To: Andrew Rybchenko , 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-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?