From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <tomaszx.kulasek@intel.com>
Received: from mga05.intel.com (mga05.intel.com [192.55.52.43])
by dpdk.org (Postfix) with ESMTP id 5084537B8
for <dev@dpdk.org>; Fri, 9 Dec 2016 14:25:13 +0100 (CET)
Received: from fmsmga001.fm.intel.com ([10.253.24.23])
by fmsmga105.fm.intel.com with ESMTP; 09 Dec 2016 05:25:12 -0800
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.33,324,1477983600"; d="scan'208";a="1079703389"
Received: from irsmsx154.ger.corp.intel.com ([163.33.192.96])
by fmsmga001.fm.intel.com with ESMTP; 09 Dec 2016 05:25:11 -0800
Received: from irsmsx102.ger.corp.intel.com ([169.254.2.79]) by
IRSMSX154.ger.corp.intel.com ([169.254.12.108]) with mapi id 14.03.0248.002;
Fri, 9 Dec 2016 13:25:10 +0000
From: "Kulasek, TomaszX" <tomaszx.kulasek@intel.com>
To: Thomas Monjalon <thomas.monjalon@6wind.com>
CC: "dev@dpdk.org" <dev@dpdk.org>, "Ananyev, Konstantin"
<konstantin.ananyev@intel.com>, "olivier.matz@6wind.com"
<olivier.matz@6wind.com>, "Richardson, Bruce" <bruce.richardson@intel.com>
Thread-Topic: [dpdk-dev] [PATCH v12 1/6] ethdev: add Tx preparation
Thread-Index: AQHSRbBkXBK52xh5bEGv2Yak9bo9p6DuQDCAgAUTIYCAACxPUIAADdsAgAAl19CAAByzgIAL4hdw
Date: Fri, 9 Dec 2016 13:25:09 +0000
Message-ID: <3042915272161B4EB253DA4D77EB373A14F5A2D0@IRSMSX102.ger.corp.intel.com>
References: <1477486575-25148-1-git-send-email-tomaszx.kulasek@intel.com>
<2505996.o0gdCe9Hsd@xps13>
<3042915272161B4EB253DA4D77EB373A14F57CDE@IRSMSX102.ger.corp.intel.com>
<4969291.OX96oIJoy2@xps13>
In-Reply-To: <4969291.OX96oIJoy2@xps13>
Accept-Language: en-US
Content-Language: en-US
X-MS-Has-Attach:
X-MS-TNEF-Correlator:
x-originating-ip: [163.33.239.180]
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Subject: Re: [dpdk-dev] [PATCH v12 1/6] ethdev: add Tx preparation
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <http://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: <http://dpdk.org/ml/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Fri, 09 Dec 2016 13:25:13 -0000
Hi Thomas,
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Friday, December 2, 2016 00:51
> To: Kulasek, TomaszX <tomaszx.kulasek@intel.com>
> Cc: dev@dpdk.org; Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> olivier.matz@6wind.com; Richardson, Bruce <bruce.richardson@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v12 1/6] ethdev: add Tx preparation
>=20
> 2016-12-01 22:31, Kulasek, TomaszX:
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > 2016-12-01 19:20, Kulasek, TomaszX:
> > > > Hi Thomas,
> > > >
> > > > Sorry, I have answered for this question in another thread and I
> > > > missed
> > > about this one. Detailed answer is below.
> > >
> > > Yes you already gave this answer.
> > > And I will continue asking the question until you understand it.
> > >
> > > > > 2016-11-28 11:54, Thomas Monjalon:
> > > > > > Hi,
> > > > > >
> > > > > > 2016-11-23 18:36, Tomasz Kulasek:
> > > > > > > --- a/config/common_base
> > > > > > > +++ b/config/common_base
> > > > > > > @@ -120,6 +120,7 @@ CONFIG_RTE_MAX_QUEUES_PER_PORT=3D1024
> > > > > > > CONFIG_RTE_LIBRTE_IEEE1588=3Dn
> > > > > > > CONFIG_RTE_ETHDEV_QUEUE_STAT_CNTRS=3D16
> > > > > > > CONFIG_RTE_ETHDEV_RXTX_CALLBACKS=3Dy
> > > > > > > +CONFIG_RTE_ETHDEV_TX_PREPARE=3Dy
> > > > > >
> > > > > > Please, remind me why is there a configuration here.
> > > > > > It should be the responsibility of the application to call
> > > > > > tx_prepare or not. If the application choose to use this new
> > > > > > API but it is disabled, then the packets won't be prepared and
> > > > > > there is
> > > no error code:
> > > > > >
> > > > > > > +#else
> > > > > > > +
> > > > > > > +static inline uint16_t
> > > > > > > +rte_eth_tx_prepare(__rte_unused uint8_t port_id,
> > > > > > > +__rte_unused
> > > > > uint16_t queue_id,
> > > > > > > + __rte_unused struct rte_mbuf **tx_pkts,
> > > > > > > +uint16_t
> > > > > > > +nb_pkts) {
> > > > > > > + return nb_pkts;
> > > > > > > +}
> > > > > > > +
> > > > > > > +#endif
> > > > > >
> > > > > > So the application is not aware of the issue and it will not
> > > > > > use any fallback.
> > > >
> > > > tx_prepare mechanism can be turned off by compilation flag (as
> > > > discussed
> > > with Jerin in http://dpdk.org/dev/patchwork/patch/15770/) to provide
> > > real NOOP functionality (e.g. for low-end CPUs, where even
> > > unnecessary memory dereference and check can have significant impact
> on performance).
> > > >
> > > > Jerin observed that on some architectures (e.g. low-end ARM with
> > > embedded NIC), just reading and comparing 'dev->tx_pkt_prepare' may
> > > cause significant performance drop, so he proposed to introduce this
> > > configuration flag to provide real NOOP when tx_prepare
> > > functionality is not required, and can be turned on based on the
> _target_ configuration.
> > > >
> > > > For other cases, when this flag is turned on (by default), and
> > > tx_prepare is not implemented, functional NOOP is used based on
> > > comparison (dev->tx_pkt_prepare =3D=3D NULL).
> > >
> > > So if the application call this function and if it is disabled, it
> > > simply won't work. Packets won't be prepared, checksum won't be
> computed.
> > >
> > > I give up, I just NACK.
> >
> > It is not to be turned on/off whatever someone wants, but only and only
> for the case, when platform developer knows, that his platform doesn't
> need this callback, so, he may turn off it and then save some performance
> (this option is per target).
>=20
> How may he know? There is no comment in the config file, no documentation=
.
>=20
> > For this case, the behavior of tx_prepare will be exactly the same when
> it is turned on or off. If is not the same, there's no sense to turn it
> off. There were long topic, where we've tried to convince you, that it
> should be turned on for all devices.
>=20
> Really? You tried to convince me to turn it on?
> No you were trying to convince Jerin.
> I think it is a wrong idea to allow disabling this function.
> I didn't comment in first discussion because Jerin told it was really
> important for small hardware with fixed NIC, and I thought it would be
> implemented in a way the application cannot be misleaded.
>=20
> The only solution I see here is to add some comments in the configuration
> file, below the #else and in the doc.
> Have you checked doc/guides/prog_guide/poll_mode_drv.rst?
I can change the name of CONFIG_RTE_ETHDEV_TX_PREPARE=3Dy to something like=
CONFIG_RTE_ETHDEV_TX_PREPARE_NOOP=3Dn to made it less confusing, and add c=
omments to describe why it is introduced and how it use safely.
I can also remove it at all if you don't like it.
As for doc/guides/prog_guide/poll_mode_drv.rst, do you mean, to add new sec=
tion describing this feature?
Tomasz