From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <ophirmu@mellanox.com>
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 <dev@dpdk.org>; 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 <ophirmu@mellanox.com>
To: Ferruh Yigit <ferruh.yigit@intel.com>, Pascal Mazon
<pascal.mazon@6wind.com>
CC: "dev@dpdk.org" <dev@dpdk.org>, Mordechay Haimovsky <motih@mellanox.com>,
Olga Shern <olgas@mellanox.com>, Thomas Monjalon <thomas@monjalon.net>,
Raslan Darawsheh <rasland@mellanox.com>, Shahaf Shuler <shahafs@mellanox.com>
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: <HE1PR0501MB2314927B82A257D6BA943074D18F0@HE1PR0501MB2314.eurprd05.prod.outlook.com>
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: <HE1PR0501MB2539AE3FA7E3A096076DC4E2D18F0@HE1PR0501MB2539.eurprd05.prod.outlook.com>
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 <dev.dpdk.org>
List-Unsubscribe: <https://dpdk.org/ml/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://dpdk.org/ml/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=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 <pascal.mazon@6wind.com>
> Cc: dev@dpdk.org; Ferruh Yigit <ferruh.yigit@intel.com>; Mordechay
> Haimovsky <motih@mellanox.com>; Ophir Munk <ophirmu@mellanox.com>
> 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 <ferruh.yigit@intel.com>
> ---
> Cc: Ophir Munk <ophirmu@mellanox.com>
>=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