From mboxrd@z Thu Jan 1 00:00:00 1970 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) X-Virus-Scanned: Proofpoint Essentials engine Received: from webmail.solarflare.com (uk.solarflare.com [193.34.186.16]) (using TLSv1 with cipher ECDHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1-us4.ppe-hosted.com (Proofpoint Essentials ESMTP Server) with ESMTPS id 103D4BC006B; Wed, 4 Jul 2018 12:52:39 +0000 (UTC) Received: from [192.168.38.17] (91.220.146.112) by ukex01.SolarFlarecom.com (10.17.10.4) with Microsoft SMTP Server (TLS) id 15.0.1044.25; Wed, 4 Jul 2018 13:52:32 +0100 To: Thomas Monjalon , CC: , , , , References: <1571938.317irMz1sZ@xps> <20180702212750.16758-4-thomas@monjalon.net> <1d842244-297a-1e0d-c86c-989a99b6c3c8@solarflare.com> <2074528.iGUK7qq6Nh@xps> From: Andrew Rybchenko Message-ID: <16e64db5-eec2-c00b-4f32-312681cb3ad6@solarflare.com> Date: Wed, 4 Jul 2018 15:52:25 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0 MIME-Version: 1.0 In-Reply-To: <2074528.iGUK7qq6Nh@xps> Content-Language: en-US X-Originating-IP: [91.220.146.112] X-ClientProxiedBy: ocex03.SolarFlarecom.com (10.20.40.36) To ukex01.SolarFlarecom.com (10.17.10.4) X-TM-AS-Product-Ver: SMEX-11.0.0.1191-8.100.1062-23946.003 X-TM-AS-Result: No--19.998200-0.000000-31 X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-MDID: 1530708760-Ck6piDKwd5bU Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-Content-Filtered-By: Mailman/MimeDel 2.1.15 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:52:41 -0000 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...