From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Rybchenko Subject: Re: [PATCH v2 3/5] ethdev: convert remaining apps to new offload API Date: Wed, 4 Jul 2018 15:52:25 +0300 Message-ID: <16e64db5-eec2-c00b-4f32-312681cb3ad6@solarflare.com> References: <1571938.317irMz1sZ@xps> <20180702212750.16758-4-thomas@monjalon.net> <1d842244-297a-1e0d-c86c-989a99b6c3c8@solarflare.com> <2074528.iGUK7qq6Nh@xps> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Cc: , , , , To: Thomas Monjalon , Return-path: Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com [148.163.129.52]) by dpdk.org (Postfix) with ESMTP id D815E1BF1E for ; Wed, 4 Jul 2018 14:52:40 +0200 (CEST) In-Reply-To: <2074528.iGUK7qq6Nh@xps> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 07/04/2018 03:26 PM, Thomas Monjalon wrote: > 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. OK. I see. Most likely yes. I agree. >>> --- 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? I have no strong opinion. As far as I can see there is no maintainer for it...