From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Monjalon Subject: Re: [PATCH v2 3/5] ethdev: convert remaining apps to new offload API Date: Wed, 04 Jul 2018 14:26:58 +0200 Message-ID: <2074528.iGUK7qq6Nh@xps> References: <1571938.317irMz1sZ@xps> <20180702212750.16758-4-thomas@monjalon.net> <1d842244-297a-1e0d-c86c-989a99b6c3c8@solarflare.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7Bit Cc: shahafs@mellanox.com, dev@dpdk.org, ravi1.kumar@amd.com, rasesh.mody@cavium.com, maxime.coquelin@redhat.com To: Andrew Rybchenko , ferruh.yigit@intel.com 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) In-Reply-To: <1d842244-297a-1e0d-c86c-989a99b6c3c8@solarflare.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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?