From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from EUR01-DB5-obe.outbound.protection.outlook.com (mail-db5eur01on0066.outbound.protection.outlook.com [104.47.2.66]) by dpdk.org (Postfix) with ESMTP id 6754B3DC for ; Wed, 25 Apr 2018 11:18:07 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=Mellanox.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version; bh=UE2u6lc0hqlJBtwpCV6fIRcrAE3py6uNtjQ85igOmXw=; b=a1N9KWesiPFFQ9d9y3nc4gINBFGpZBpfMX7SRnRWEZ7rsTqJvSzJOnK/7Ufk08pb0tLjQCHD+IynTcfyMVd2ZUDmtOG1oVIpdnTxxKvVz2j2aEpTIe/ic/72d/KygUBY8i2WoqfqjHs0euM76Ac9cNIrCmcsN1ZbcAb7RVE/onY= Received: from HE1PR0501MB2314.eurprd05.prod.outlook.com (10.168.34.19) by HE1PR0501MB2539.eurprd05.prod.outlook.com (10.168.126.147) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P256) id 15.20.696.12; Wed, 25 Apr 2018 09:18:04 +0000 Received: from HE1PR0501MB2314.eurprd05.prod.outlook.com ([fe80::d405:aec8:cd2f:85cc]) by HE1PR0501MB2314.eurprd05.prod.outlook.com ([fe80::d405:aec8:cd2f:85cc%18]) with mapi id 15.20.0675.018; Wed, 25 Apr 2018 09:18:04 +0000 From: Ophir Munk To: Ferruh Yigit , Pascal Mazon CC: "dev@dpdk.org" , Mordechay Haimovsky , Olga Shern , Thomas Monjalon , Raslan Darawsheh , Shahaf Shuler Thread-Topic: [PATCH v3] net/tap: remove queue specific offload support Thread-Index: AQHT2/VMztHsJDV5x06BviJMlkSv5qQRGsxA Date: Wed, 25 Apr 2018 09:18:04 +0000 Message-ID: References: <20180423093846.81133-1-ferruh.yigit@intel.com> <20180424175408.42099-1-ferruh.yigit@intel.com> In-Reply-To: <20180424175408.42099-1-ferruh.yigit@intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=ophirmu@mellanox.com; x-originating-ip: [193.47.165.251] x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1; HE1PR0501MB2539; 7:BGl4QyFxMj1mtt5q9ZmZhRHUjYjhb/I97aPSiAuPsvAaBCNqLQmlNGX+5nUx4EE82LQO39EyKXYAG9kBF60wLPg6q0xtPOjzTN4OT/mmbsPW4hvb89nKncBQRqWs+y8c1COhHA2f/enmtAZgztv0XP2HKXCBWOvVx7l2Mc6mP1+fHJip5aw1BOHQQyJSO/J+LT5JYzBrO3S2McD1gmXhGBj9R1QtH7Nr75mk/ckqroAZ3sTQ8gyD2euELp/SQn+m x-ms-exchange-antispam-srfa-diagnostics: SOS; x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: UriScan:; BCL:0; PCL:0; RULEID:(7020095)(4652020)(48565401081)(5600026)(4534165)(4627221)(201703031133081)(201702281549075)(2017052603328)(7153060)(7193020); SRVR:HE1PR0501MB2539; x-ms-traffictypediagnostic: HE1PR0501MB2539: x-ld-processed: a652971c-7d2e-4d9b-a6a4-d149256f461b,ExtAddr x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(278428928389397)(228905959029699); x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(8211001083)(6040522)(2401047)(8121501046)(5005006)(10201501046)(93006095)(93001095)(3231232)(944501410)(52105095)(3002001)(6055026)(6041310)(20161123560045)(20161123558120)(20161123562045)(20161123564045)(201703131423095)(201702281528075)(20161123555045)(201703061421075)(201703061406153)(6072148)(201708071742011); SRVR:HE1PR0501MB2539; BCL:0; PCL:0; RULEID:; SRVR:HE1PR0501MB2539; x-forefront-prvs: 06530126A4 x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(39860400002)(39380400002)(376002)(366004)(396003)(346002)(199004)(189003)(13464003)(5660300001)(446003)(76176011)(105586002)(106356001)(6116002)(3846002)(8936002)(3280700002)(8676002)(229853002)(486006)(33656002)(66066001)(14454004)(7736002)(476003)(81166006)(3660700001)(86362001)(2906002)(11346002)(81156014)(5250100002)(186003)(316002)(25786009)(107886003)(6246003)(6436002)(102836004)(74316002)(68736007)(97736004)(26005)(110136005)(54906003)(478600001)(55016002)(305945005)(9686003)(4326008)(53936002)(99286004)(53546011)(2900100001)(7696005)(6506007)(59450400001); DIR:OUT; SFP:1101; SCL:1; SRVR:HE1PR0501MB2539; H:HE1PR0501MB2314.eurprd05.prod.outlook.com; FPR:; SPF:None; LANG:en; PTR:InfoNoRecords; A:1; MX:1; received-spf: None (protection.outlook.com: mellanox.com does not designate permitted sender hosts) x-microsoft-antispam-message-info: DU2HkD9ZdmJWQz3XNoW0RSsiIAV5TeUW91bTejhpaw8nYrQK/Gsg+ZLzA4rTd6eH22b18qj/xo21qGenUn2cpOWV7QEzyUJIpE8dAJXc1lVaecDrbD1ANeFch5nKXZXTlZhqzFodfPdjSePR/yYzuHlDfhxVP4bqsB1ojOp/PZzAcdjvSPJeXwf86tc+TLnJ spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-MS-Office365-Filtering-Correlation-Id: e45bb51f-8ba6-47a8-417e-08d5aa8d735a X-OriginatorOrg: Mellanox.com X-MS-Exchange-CrossTenant-Network-Message-Id: e45bb51f-8ba6-47a8-417e-08d5aa8d735a X-MS-Exchange-CrossTenant-originalarrivaltime: 25 Apr 2018 09:18:04.3636 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: a652971c-7d2e-4d9b-a6a4-d149256f461b X-MS-Exchange-Transport-CrossTenantHeadersStamped: HE1PR0501MB2539 Subject: Re: [dpdk-dev] [PATCH v3] net/tap: remove queue specific offload support 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, 25 Apr 2018 09:18:07 -0000 Hi Ferruh, I should have mentioned earlier that TAP does support queue specific capabi= lities.=20 Please look in tap_queue_setup() and note that each TAP queue is created wi= th a distinct file descriptor (fd). Then supporting an offload capability is just implementing it in SW (e.g. c= alculating IP checksum). If the main assumption of this patch was that TAP does not support queue sp= ecific offloads - then please consider this patch again. On the other hand there is no port specific capability supported by TAP. Ho= wever, in order to support legacy applications, port capabilities are usual= ly reported as the OR operation between queue & port capabilities.=20 TAP currently clones the queue capabilities to port capabilities. We could = optimize this cloning by always return queue capabilities when queried abou= t queues or ports. In this case - tap_rx_offload_get_port_capa() and tap_tx= _offload_get_port_capa() could be removed. Please find more comments inline. > -----Original Message----- > From: Ferruh Yigit [mailto:ferruh.yigit@intel.com] > Sent: Tuesday, April 24, 2018 8:54 PM > To: Pascal Mazon > Cc: dev@dpdk.org; Ferruh Yigit ; Mordechay > Haimovsky ; Ophir Munk > Subject: [PATCH v3] net/tap: remove queue specific offload support >=20 > It is not clear if tap PMD supports queue specific offloads, removing the > related code. >=20 > Fixes: 95ae196ae10b ("net/tap: use new Rx offloads API") > Fixes: 818fe14a9891 ("net/tap: use new Tx offloads API") > Cc: motih@mellanox.com >=20 > Signed-off-by: Ferruh Yigit > --- > Cc: Ophir Munk >=20 > v2: > * rebased >=20 > v3: > * txq->csum restored, > - ETH_TXQ_FLAGS_IGNORE check removed since ethdev layer takes care of > it > - tx_conf !=3D NULL check removed, this is internal api who calls this = is > ethdev and it doesn't pass null tx_conf > --- > drivers/net/tap/rte_eth_tap.c | 102 +++++-------------------------------= ------ > 1 file changed, 10 insertions(+), 92 deletions(-) >=20 > diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.= c > index ef33aace9..61b4b5df3 100644 > --- a/drivers/net/tap/rte_eth_tap.c > +++ b/drivers/net/tap/rte_eth_tap.c > @@ -278,31 +278,6 @@ tap_rx_offload_get_port_capa(void) > DEV_RX_OFFLOAD_CRC_STRIP; > } >=20 > -static uint64_t > -tap_rx_offload_get_queue_capa(void) > -{ > - return DEV_RX_OFFLOAD_SCATTER | > - DEV_RX_OFFLOAD_IPV4_CKSUM | > - DEV_RX_OFFLOAD_UDP_CKSUM | > - DEV_RX_OFFLOAD_TCP_CKSUM | > - DEV_RX_OFFLOAD_CRC_STRIP; > -} > - TAP PMD supports all of these RX queue specific offloads. I suggest to leav= e this function in place. > -static bool > -tap_rxq_are_offloads_valid(struct rte_eth_dev *dev, uint64_t offloads) -= { > - uint64_t port_offloads =3D dev->data->dev_conf.rxmode.offloads; > - uint64_t queue_supp_offloads =3D tap_rx_offload_get_queue_capa(); > - uint64_t port_supp_offloads =3D tap_rx_offload_get_port_capa(); > - > - if ((offloads & (queue_supp_offloads | port_supp_offloads)) !=3D > - offloads) > - return false; > - if ((port_offloads ^ offloads) & port_supp_offloads) > - return false; > - return true; > -} > - Putting aside the fact that queue offloads equals port offloads (so could i= gnore "port_supp_offload" variable) - this function is essential to validat= e that the configured Rx offloads are supported by TAP. I suggest to leave = this function in place. Without it - testpmd falsely confirms non supported offloads. For example before this patch: offloading "hw-vlan-filter" will fail as exp= ected: testpmd> port config all testpmd> port config all hw-vlan-filter on testpmd> port start all Configuring Port 0 (socket 0) PMD: net_tap0: 0x1209fc0: TX configured queues number: 1 PMD: net_tap0: 0x1209fc0: RX configured queues number: 1 PMD: 0x1209fc0: Rx queue offloads 0x120e don't match port offloads 0x120e o= r supported offloads 0x300e Fail to configure port 0 rx queues However, with this patch this configuration is falsely accepted. > /* Callback to handle the rx burst of packets to the correct interface a= nd > * file descriptor(s) in a multi-queue setup. > */ > @@ -411,31 +386,6 @@ tap_tx_offload_get_port_capa(void) > DEV_TX_OFFLOAD_TCP_CKSUM; > } >=20 > -static uint64_t > -tap_tx_offload_get_queue_capa(void) > -{ > - return DEV_TX_OFFLOAD_MULTI_SEGS | > - DEV_TX_OFFLOAD_IPV4_CKSUM | > - DEV_TX_OFFLOAD_UDP_CKSUM | > - DEV_TX_OFFLOAD_TCP_CKSUM; > -} > - TAP PMD supports all of these TX queue specific offloads. I suggest to leav= e this function in place. > -static bool > -tap_txq_are_offloads_valid(struct rte_eth_dev *dev, uint64_t offloads) -= { > - uint64_t port_offloads =3D dev->data->dev_conf.txmode.offloads; > - uint64_t queue_supp_offloads =3D tap_tx_offload_get_queue_capa(); > - uint64_t port_supp_offloads =3D tap_tx_offload_get_port_capa(); > - > - if ((offloads & (queue_supp_offloads | port_supp_offloads)) !=3D > - offloads) > - return false; > - /* Verify we have no conflict with port offloads */ > - if ((port_offloads ^ offloads) & port_supp_offloads) > - return false; > - return true; > -} > - This function is essential to validate that the configured Tx offloads are = supported by TAP.=20 I suggest to leave this function in place. > static void > tap_tx_offload(char *packet, uint64_t ol_flags, unsigned int l2_len, > unsigned int l3_len) > @@ -763,12 +713,10 @@ tap_dev_info(struct rte_eth_dev *dev, struct > rte_eth_dev_info *dev_info) > dev_info->max_tx_queues =3D RTE_PMD_TAP_MAX_QUEUES; > dev_info->min_rx_bufsize =3D 0; > dev_info->speed_capa =3D tap_dev_speed_capa(); > - dev_info->rx_queue_offload_capa =3D > tap_rx_offload_get_queue_capa(); > - dev_info->rx_offload_capa =3D tap_rx_offload_get_port_capa() | > - dev_info->rx_queue_offload_capa; > - dev_info->tx_queue_offload_capa =3D > tap_tx_offload_get_queue_capa(); > - dev_info->tx_offload_capa =3D tap_tx_offload_get_port_capa() | > - dev_info->tx_queue_offload_capa; > + dev_info->rx_offload_capa =3D tap_rx_offload_get_port_capa(); > + dev_info->tx_offload_capa =3D tap_tx_offload_get_port_capa(); > + dev_info->rx_queue_offload_capa =3D 0; > + dev_info->tx_queue_offload_capa =3D 0; > } >=20 Rx_queue_offloads_capa should be reported as before: dev_info->tx_queue_offload_capa =3D tap_tx_offload_get_queue_capa(); Same for TX offloads. Port capabilities could return queue capabilities: Instead of: dev_info->rx_offload_capa =3D tap_rx_offload_get_port_capa() | dev_info->rx_queue_offload_capa; We could return: dev_info->rx_offload_capa =3D dev_info->rx_queue_offload_capa; The same argument is valid for Tx as well. > static int > @@ -1094,19 +1042,6 @@ tap_rx_queue_setup(struct rte_eth_dev *dev, > return -1; > } >=20 > - /* Verify application offloads are valid for our port and queue. */ > - if (!tap_rxq_are_offloads_valid(dev, rx_conf->offloads)) { > - rte_errno =3D ENOTSUP; > - RTE_LOG(ERR, PMD, > - "%p: Rx queue offloads 0x%" PRIx64 > - " don't match port offloads 0x%" PRIx64 > - " or supported offloads 0x%" PRIx64 "\n", > - (void *)dev, rx_conf->offloads, > - dev->data->dev_conf.rxmode.offloads, > - (tap_rx_offload_get_port_capa() | > - tap_rx_offload_get_queue_capa())); > - return -rte_errno; > - } The tap_rxq_are_offloads_valid() call is essential. I suggest to leave it i= n place. The RTE_LOG could drop port references to become: RTE_LOG(ERR, PMD, "%p: Rx queue offloads 0x%" PRIx64 " don't match" " supported offloads 0x%" PRIx64 "\n", (void *)dev, rx_conf->offloads, tap_rx_offload_get_queue_capa())); > rxq->mp =3D mp; > rxq->trigger_seen =3D 1; /* force initial burst */ > rxq->in_port =3D dev->data->port_id; > @@ -1175,29 +1110,12 @@ tap_tx_queue_setup(struct rte_eth_dev *dev, > return -1; > dev->data->tx_queues[tx_queue_id] =3D &internals->txq[tx_queue_id]; > txq =3D dev->data->tx_queues[tx_queue_id]; > - /* > - * Don't verify port offloads for application which > - * use the old API. > - */ > - if (tx_conf !=3D NULL && > - !!(tx_conf->txq_flags & ETH_TXQ_FLAGS_IGNORE)) { > - if (tap_txq_are_offloads_valid(dev, tx_conf->offloads)) { > - txq->csum =3D !!(tx_conf->offloads & > - (DEV_TX_OFFLOAD_IPV4_CKSUM | > - DEV_TX_OFFLOAD_UDP_CKSUM | > - DEV_TX_OFFLOAD_TCP_CKSUM)); > - } else { > - rte_errno =3D ENOTSUP; > - RTE_LOG(ERR, PMD, > - "%p: Tx queue offloads 0x%" PRIx64 > - " don't match port offloads 0x%" PRIx64 > - " or supported offloads 0x%" PRIx64, > - (void *)dev, tx_conf->offloads, > - dev->data->dev_conf.txmode.offloads, > - tap_tx_offload_get_port_capa()); > - return -rte_errno; > - } > - } > + The tap_txq_are_offloads_valid() call is essential. I suggest to leave it i= n place. The RTE_LOG message could drop comparison between queue and port capabiliti= es: RTE_LOG(ERR, PMD, "%p: Tx queue offloads 0x%" PRIx64 " don't match" " supported offloads 0x%" PRIx64, (void *)dev, tx_conf->offloads, tap_tx_offload_get_queue_capa()); > + txq->csum =3D !!(tx_conf->offloads & > + (DEV_TX_OFFLOAD_IPV4_CKSUM | > + DEV_TX_OFFLOAD_UDP_CKSUM | > + DEV_TX_OFFLOAD_TCP_CKSUM)); > + > ret =3D tap_setup_queue(dev, internals, tx_queue_id, 0); > if (ret =3D=3D -1) > return -1; > -- > 2.14.3