Re: [dpdk-dev] [PATCH v12 1/6] ethdev: add Tx preparation
> > 2016-12-12 11:51, Ananyev, Konstantin: > > > > The application gets few information from tx_prepare() about what should > > > > be done to make the packet accepted by the hw, and the actions will > > > > probably be different depending on hardware. > > > > That's true. > > I am open to suggestions how in future to provide extra information to the > > upper layer. > > Set rte_errno to different values depending on type of error, > > OR extra parameter in tx_prepare() that will provide more detailed error > > information, > > OR something else? > > That's one of the reason which give me a feeling that it is safer > to introduce tx_prepare as an experimental API in 17.02. > So the users will know that it can change in the next release. > What do you think? I think that's the good reason and I am ok with it. Konstantin
Re: [dpdk-dev] [PATCH v12 1/6] ethdev: add Tx preparation
On Thu, Dec 22, 2016 at 02:14:45PM +0100, Thomas Monjalon wrote: > 2016-12-02 00:10, Ananyev, Konstantin: > > I have absolutely no problem to remove the RTE_ETHDEV_TX_PREPARE and > > associated logic. > > I personally don't use ARM boxes and don't plan to, > > and in theory users can still do conditional compilation at the upper > > layer, if they want to. > > Yes you're right. The application can avoid calling tx_prepare at all. There are applications inside dpdk repo which will be using tx_prep so in that case, IMHO, let the ifdef inside the DPDK library and disable it by default so that if required we can disable it in one shot on integrated controllers targets where is the system has only one integrated controller and integrated controller does not need tx_prep > No need of an ifdef inside DPDK.
Re: [dpdk-dev] [PATCH v12 1/6] ethdev: add Tx preparation
2016-12-12 11:51, Ananyev, Konstantin: > > > The application gets few information from tx_prepare() about what should > > > be done to make the packet accepted by the hw, and the actions will > > > probably be different depending on hardware. > > That's true. > I am open to suggestions how in future to provide extra information to the > upper layer. > Set rte_errno to different values depending on type of error, > OR extra parameter in tx_prepare() that will provide more detailed error > information, > OR something else? That's one of the reason which give me a feeling that it is safer to introduce tx_prepare as an experimental API in 17.02. So the users will know that it can change in the next release. What do you think?
Re: [dpdk-dev] [PATCH v12 1/6] ethdev: add Tx preparation
2016-12-02 00:10, Ananyev, Konstantin: > I have absolutely no problem to remove the RTE_ETHDEV_TX_PREPARE and > associated logic. > I personally don't use ARM boxes and don't plan to, > and in theory users can still do conditional compilation at the upper layer, > if they want to. Yes you're right. The application can avoid calling tx_prepare at all. No need of an ifdef inside DPDK.
Re: [dpdk-dev] [PATCH v12 1/6] ethdev: add Tx preparation
Hi Olivier and Tomasz, > -Original Message- > From: Kulasek, TomaszX > Sent: Friday, December 9, 2016 5:19 PM > To: Olivier Matz ; Ananyev, Konstantin > > Cc: Thomas Monjalon ; dev@dpdk.org > Subject: RE: [dpdk-dev] [PATCH v12 1/6] ethdev: add Tx preparation > > Hi Oliver, > > My 5 cents below: > > > -Original Message- > > From: Olivier Matz [mailto:olivier.m...@6wind.com] > > Sent: Thursday, December 8, 2016 18:24 > > To: Ananyev, Konstantin > > Cc: Thomas Monjalon ; Kulasek, TomaszX > > ; dev@dpdk.org > > Subject: Re: [dpdk-dev] [PATCH v12 1/6] ethdev: add Tx preparation > > > > Hi Konstantin, > > > > On Fri, 2 Dec 2016 16:17:51 +, "Ananyev, Konstantin" > > wrote: > > > Hi Olivier, > > > > > > > -Original Message- > > > > From: Olivier Matz [mailto:olivier.m...@6wind.com] > > > > Sent: Friday, December 2, 2016 8:24 AM > > > > To: Ananyev, Konstantin > > > > Cc: Thomas Monjalon ; Kulasek, TomaszX > > > > ; dev@dpdk.org Subject: Re: [dpdk-dev] > > > > [PATCH v12 1/6] ethdev: add Tx preparation > > > > > > > > Hi Konstantin, > > > > > > > > On Fri, 2 Dec 2016 01:06:30 +, "Ananyev, Konstantin" > > > > wrote: > > > > > > > > > > > > 2016-11-23 18:36, Tomasz Kulasek: > > > > > > > +/** > > > > > > > + * Process a burst of output packets on a transmit queue of > > > > > > > an Ethernet device. > > > > > > > + * > > > > > > > + * The rte_eth_tx_prepare() function is invoked to prepare > > > > > > > output packets to be > > > > > > > + * transmitted on the output queue *queue_id* of the Ethernet > > > > > > > device designated > > > > > > > + * by its *port_id*. > > > > > > > + * The *nb_pkts* parameter is the number of packets to be > > > > > > > prepared which are > > > > > > > + * supplied in the *tx_pkts* array of *rte_mbuf* structures, > > > > > > > each of them > > > > > > > + * allocated from a pool created with > > > > > > > rte_pktmbuf_pool_create(). > > > > > > > + * For each packet to send, the rte_eth_tx_prepare() function > > > > > > > performs > > > > > > > + * the following operations: > > > > > > > + * > > > > > > > + * - Check if packet meets devices requirements for tx > > > > > > > offloads. > > > > > > > + * > > > > > > > + * - Check limitations about number of segments. > > > > > > > + * > > > > > > > + * - Check additional requirements when debug is enabled. > > > > > > > + * > > > > > > > + * - Update and/or reset required checksums when tx offload > > > > > > > is set for packet. > > > > > > > + * > > > > > > > + * Since this function can modify packet data, provided mbufs > > > > > > > must be safely > > > > > > > + * writable (e.g. modified data cannot be in shared > > > > > > > segment). > > > > > > > > > > > > I think we will have to remove this limitation in next releases. > > > > > > As we don't know how it could affect the API, I suggest to > > > > > > declare this API EXPERIMENTAL. > > > > > > > > > > While I don't really mind to mart it as experimental, I don't > > > > > really understand the reasoning: Why " this function can modify > > > > > packet data, provided mbufs must be safely writable" suddenly > > > > > becomes a problem? That seems like and obvious limitation to me > > > > > and let say tx_burst() has the same one. Second, I don't see how > > > > > you are going to remove it without introducing a heavy performance > > > > > impact. Konstantin > > > > > > > > About tx_burst(), I don't think we should force the user to provide > > > > a writable mbuf. There are many use cases where passing a clone > > > > already works as of today and it avoids duplicating the mbuf data. > > > > For instance: traffic generator, multicast, bridging/tap, etc... >
Re: [dpdk-dev] [PATCH v12 1/6] ethdev: add Tx preparation
Hi Oliver, My 5 cents below: > -Original Message- > From: Olivier Matz [mailto:olivier.m...@6wind.com] > Sent: Thursday, December 8, 2016 18:24 > To: Ananyev, Konstantin > Cc: Thomas Monjalon ; Kulasek, TomaszX > ; dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH v12 1/6] ethdev: add Tx preparation > > Hi Konstantin, > > On Fri, 2 Dec 2016 16:17:51 +, "Ananyev, Konstantin" > wrote: > > Hi Olivier, > > > > > -Original Message- > > > From: Olivier Matz [mailto:olivier.m...@6wind.com] > > > Sent: Friday, December 2, 2016 8:24 AM > > > To: Ananyev, Konstantin > > > Cc: Thomas Monjalon ; Kulasek, TomaszX > > > ; dev@dpdk.org Subject: Re: [dpdk-dev] > > > [PATCH v12 1/6] ethdev: add Tx preparation > > > > > > Hi Konstantin, > > > > > > On Fri, 2 Dec 2016 01:06:30 +, "Ananyev, Konstantin" > > > wrote: > > > > > > > > > > 2016-11-23 18:36, Tomasz Kulasek: > > > > > > +/** > > > > > > + * Process a burst of output packets on a transmit queue of > > > > > > an Ethernet device. > > > > > > + * > > > > > > + * The rte_eth_tx_prepare() function is invoked to prepare > > > > > > output packets to be > > > > > > + * transmitted on the output queue *queue_id* of the Ethernet > > > > > > device designated > > > > > > + * by its *port_id*. > > > > > > + * The *nb_pkts* parameter is the number of packets to be > > > > > > prepared which are > > > > > > + * supplied in the *tx_pkts* array of *rte_mbuf* structures, > > > > > > each of them > > > > > > + * allocated from a pool created with > > > > > > rte_pktmbuf_pool_create(). > > > > > > + * For each packet to send, the rte_eth_tx_prepare() function > > > > > > performs > > > > > > + * the following operations: > > > > > > + * > > > > > > + * - Check if packet meets devices requirements for tx > > > > > > offloads. > > > > > > + * > > > > > > + * - Check limitations about number of segments. > > > > > > + * > > > > > > + * - Check additional requirements when debug is enabled. > > > > > > + * > > > > > > + * - Update and/or reset required checksums when tx offload > > > > > > is set for packet. > > > > > > + * > > > > > > + * Since this function can modify packet data, provided mbufs > > > > > > must be safely > > > > > > + * writable (e.g. modified data cannot be in shared > > > > > > segment). > > > > > > > > > > I think we will have to remove this limitation in next releases. > > > > > As we don't know how it could affect the API, I suggest to > > > > > declare this API EXPERIMENTAL. > > > > > > > > While I don't really mind to mart it as experimental, I don't > > > > really understand the reasoning: Why " this function can modify > > > > packet data, provided mbufs must be safely writable" suddenly > > > > becomes a problem? That seems like and obvious limitation to me > > > > and let say tx_burst() has the same one. Second, I don't see how > > > > you are going to remove it without introducing a heavy performance > > > > impact. Konstantin > > > > > > About tx_burst(), I don't think we should force the user to provide > > > a writable mbuf. There are many use cases where passing a clone > > > already works as of today and it avoids duplicating the mbuf data. > > > For instance: traffic generator, multicast, bridging/tap, etc... > > > > > > Moreover, this requirement would be inconsistent with the model you > > > are proposing in case of pipeline: > > > - tx_prepare() on core X, may update the data > > > - tx_burst() on core Y, should not touch the data to avoid cache > > > misses > > > > Probably I wasn't very clear in my previous mail. > > I am not saying that we should force the user to pass a writable mbuf. > > What I am saying that for tx_burst() current expectation is that after > > mbuf is handled to tx_burst() user shouldn't try to modify its buffer > > contents till TX engine is done with the buffer (mbuf_free() is
Re: [dpdk-dev] [PATCH v12 1/6] ethdev: add Tx preparation
Hi Thomas,
> -Original Message-
> From: Thomas Monjalon [mailto:thomas.monja...@6wind.com]
> Sent: Friday, December 2, 2016 00:51
> To: Kulasek, TomaszX
> Cc: dev@dpdk.org; Ananyev, Konstantin ;
> olivier.m...@6wind.com; Richardson, Bruce
> Subject: Re: [dpdk-dev] [PATCH v12 1/6] ethdev: add Tx preparation
>
> 2016-12-01 22:31, Kulasek, TomaszX:
> > From: Thomas Monjalon [mailto:thomas.monja...@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=1024
> > > > > > > CONFIG_RTE_LIBRTE_IEEE1588=n
> > > > > > > CONFIG_RTE_ETHDEV_QUEUE_STAT_CNTRS=16
> > > > > > > CONFIG_RTE_ETHDEV_RXTX_CALLBACKS=y
> > > > > > > +CONFIG_RTE_ETHDEV_TX_PREPARE=y
> > > > > >
> > > > > > 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 == 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).
>
> How may he know? There is no comment in the config file, no documentation.
>
> > 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.
>
> 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.
>
> 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=y to something like
CONFIG_RTE_ETHDEV_TX_PREPARE_NOOP=n to made it less confusing, and add comments
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 section
describing this feature?
Tomasz
Re: [dpdk-dev] [PATCH v12 1/6] ethdev: add Tx preparation
Hi Konstantin, On Fri, 2 Dec 2016 16:17:51 +, "Ananyev, Konstantin" wrote: > Hi Olivier, > > > -Original Message- > > From: Olivier Matz [mailto:olivier.m...@6wind.com] > > Sent: Friday, December 2, 2016 8:24 AM > > To: Ananyev, Konstantin > > Cc: Thomas Monjalon ; Kulasek, TomaszX > > ; dev@dpdk.org Subject: Re: [dpdk-dev] > > [PATCH v12 1/6] ethdev: add Tx preparation > > > > Hi Konstantin, > > > > On Fri, 2 Dec 2016 01:06:30 +, "Ananyev, Konstantin" > > wrote: > > > > > > > > 2016-11-23 18:36, Tomasz Kulasek: > > > > > +/** > > > > > + * Process a burst of output packets on a transmit queue of > > > > > an Ethernet device. > > > > > + * > > > > > + * The rte_eth_tx_prepare() function is invoked to prepare > > > > > output packets to be > > > > > + * transmitted on the output queue *queue_id* of the Ethernet > > > > > device designated > > > > > + * by its *port_id*. > > > > > + * The *nb_pkts* parameter is the number of packets to be > > > > > prepared which are > > > > > + * supplied in the *tx_pkts* array of *rte_mbuf* structures, > > > > > each of them > > > > > + * allocated from a pool created with > > > > > rte_pktmbuf_pool_create(). > > > > > + * For each packet to send, the rte_eth_tx_prepare() function > > > > > performs > > > > > + * the following operations: > > > > > + * > > > > > + * - Check if packet meets devices requirements for tx > > > > > offloads. > > > > > + * > > > > > + * - Check limitations about number of segments. > > > > > + * > > > > > + * - Check additional requirements when debug is enabled. > > > > > + * > > > > > + * - Update and/or reset required checksums when tx offload > > > > > is set for packet. > > > > > + * > > > > > + * Since this function can modify packet data, provided mbufs > > > > > must be safely > > > > > + * writable (e.g. modified data cannot be in shared > > > > > segment). > > > > > > > > I think we will have to remove this limitation in next releases. > > > > As we don't know how it could affect the API, I suggest to > > > > declare this API EXPERIMENTAL. > > > > > > While I don't really mind to mart it as experimental, I don't > > > really understand the reasoning: Why " this function can modify > > > packet data, provided mbufs must be safely writable" suddenly > > > becomes a problem? That seems like and obvious limitation to me > > > and let say tx_burst() has the same one. Second, I don't see how > > > you are going to remove it without introducing a heavy > > > performance impact. Konstantin > > > > About tx_burst(), I don't think we should force the user to provide > > a writable mbuf. There are many use cases where passing a clone > > already works as of today and it avoids duplicating the mbuf data. > > For instance: traffic generator, multicast, bridging/tap, etc... > > > > Moreover, this requirement would be inconsistent with the model you > > are proposing in case of pipeline: > > - tx_prepare() on core X, may update the data > > - tx_burst() on core Y, should not touch the data to avoid cache > > misses > > Probably I wasn't very clear in my previous mail. > I am not saying that we should force the user to pass a writable mbuf. > What I am saying that for tx_burst() current expectation is that > after mbuf is handled to tx_burst() user shouldn't try to modify its > buffer contents till TX engine is done with the buffer (mbuf_free() > is called by TX func for it). For tx_prep(), I think, it is the same > though restrictions are a bit more strict: user should not try to > read/write to the mbuf while tx_prep() is not finished with it. What > puzzles me is that why that should be the reason to mark tx_prep() as > experimental. Konstantin To be sure we're on the same page, let me reword: - mbufs passed to tx_prepare() by the application must have their headers (l2_len + l3_len + l4_len) writable because the phdr checksum can be replaced. It could be precised in the api comment. - mbufs passed to tx_burst() must not be modified by the driver/hw, nor by the application. About the API itself, I have one more question. I know yo
Re: [dpdk-dev] [PATCH v12 1/6] ethdev: add Tx preparation
Hi Olivier, > -Original Message- > From: Olivier Matz [mailto:olivier.m...@6wind.com] > Sent: Friday, December 2, 2016 8:24 AM > To: Ananyev, Konstantin > Cc: Thomas Monjalon ; Kulasek, TomaszX > ; dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH v12 1/6] ethdev: add Tx preparation > > Hi Konstantin, > > On Fri, 2 Dec 2016 01:06:30 +, "Ananyev, Konstantin" > wrote: > > > > > > 2016-11-23 18:36, Tomasz Kulasek: > > > > +/** > > > > + * Process a burst of output packets on a transmit queue of an > > > > Ethernet device. > > > > + * > > > > + * The rte_eth_tx_prepare() function is invoked to prepare > > > > output packets to be > > > > + * transmitted on the output queue *queue_id* of the Ethernet > > > > device designated > > > > + * by its *port_id*. > > > > + * The *nb_pkts* parameter is the number of packets to be > > > > prepared which are > > > > + * supplied in the *tx_pkts* array of *rte_mbuf* structures, > > > > each of them > > > > + * allocated from a pool created with rte_pktmbuf_pool_create(). > > > > + * For each packet to send, the rte_eth_tx_prepare() function > > > > performs > > > > + * the following operations: > > > > + * > > > > + * - Check if packet meets devices requirements for tx offloads. > > > > + * > > > > + * - Check limitations about number of segments. > > > > + * > > > > + * - Check additional requirements when debug is enabled. > > > > + * > > > > + * - Update and/or reset required checksums when tx offload is > > > > set for packet. > > > > + * > > > > + * Since this function can modify packet data, provided mbufs > > > > must be safely > > > > + * writable (e.g. modified data cannot be in shared segment). > > > > > > I think we will have to remove this limitation in next releases. > > > As we don't know how it could affect the API, I suggest to declare > > > this API EXPERIMENTAL. > > > > While I don't really mind to mart it as experimental, I don't really > > understand the reasoning: Why " this function can modify packet data, > > provided mbufs must be safely writable" suddenly becomes a problem? > > That seems like and obvious limitation to me and let say tx_burst() > > has the same one. Second, I don't see how you are going to remove it > > without introducing a heavy performance impact. Konstantin > > > > About tx_burst(), I don't think we should force the user to provide a > writable mbuf. There are many use cases where passing a clone > already works as of today and it avoids duplicating the mbuf data. For > instance: traffic generator, multicast, bridging/tap, etc... > > Moreover, this requirement would be inconsistent with the model you are > proposing in case of pipeline: > - tx_prepare() on core X, may update the data > - tx_burst() on core Y, should not touch the data to avoid cache misses > Probably I wasn't very clear in my previous mail. I am not saying that we should force the user to pass a writable mbuf. What I am saying that for tx_burst() current expectation is that after mbuf is handled to tx_burst() user shouldn't try to modify its buffer contents till TX engine is done with the buffer (mbuf_free() is called by TX func for it). For tx_prep(), I think, it is the same though restrictions are a bit more strict: user should not try to read/write to the mbuf while tx_prep() is not finished with it. What puzzles me is that why that should be the reason to mark tx_prep() as experimental. Konstantin
Re: [dpdk-dev] [PATCH v12 1/6] ethdev: add Tx preparation
Hi Konstantin, On Fri, 2 Dec 2016 01:06:30 +, "Ananyev, Konstantin" wrote: > > > > 2016-11-23 18:36, Tomasz Kulasek: > > > +/** > > > + * Process a burst of output packets on a transmit queue of an > > > Ethernet device. > > > + * > > > + * The rte_eth_tx_prepare() function is invoked to prepare > > > output packets to be > > > + * transmitted on the output queue *queue_id* of the Ethernet > > > device designated > > > + * by its *port_id*. > > > + * The *nb_pkts* parameter is the number of packets to be > > > prepared which are > > > + * supplied in the *tx_pkts* array of *rte_mbuf* structures, > > > each of them > > > + * allocated from a pool created with rte_pktmbuf_pool_create(). > > > + * For each packet to send, the rte_eth_tx_prepare() function > > > performs > > > + * the following operations: > > > + * > > > + * - Check if packet meets devices requirements for tx offloads. > > > + * > > > + * - Check limitations about number of segments. > > > + * > > > + * - Check additional requirements when debug is enabled. > > > + * > > > + * - Update and/or reset required checksums when tx offload is > > > set for packet. > > > + * > > > + * Since this function can modify packet data, provided mbufs > > > must be safely > > > + * writable (e.g. modified data cannot be in shared segment). > > > > I think we will have to remove this limitation in next releases. > > As we don't know how it could affect the API, I suggest to declare > > this API EXPERIMENTAL. > > While I don't really mind to mart it as experimental, I don't really > understand the reasoning: Why " this function can modify packet data, > provided mbufs must be safely writable" suddenly becomes a problem? > That seems like and obvious limitation to me and let say tx_burst() > has the same one. Second, I don't see how you are going to remove it > without introducing a heavy performance impact. Konstantin > About tx_burst(), I don't think we should force the user to provide a writable mbuf. There are many use cases where passing a clone already works as of today and it avoids duplicating the mbuf data. For instance: traffic generator, multicast, bridging/tap, etc... Moreover, this requirement would be inconsistent with the model you are proposing in case of pipeline: - tx_prepare() on core X, may update the data - tx_burst() on core Y, should not touch the data to avoid cache misses Regards, Olivier
Re: [dpdk-dev] [PATCH v12 1/6] ethdev: add Tx preparation
> > 2016-11-23 18:36, Tomasz Kulasek: > > +/** > > + * Process a burst of output packets on a transmit queue of an Ethernet > > device. > > + * > > + * The rte_eth_tx_prepare() function is invoked to prepare output packets > > to be > > + * transmitted on the output queue *queue_id* of the Ethernet device > > designated > > + * by its *port_id*. > > + * The *nb_pkts* parameter is the number of packets to be prepared which > > are > > + * supplied in the *tx_pkts* array of *rte_mbuf* structures, each of them > > + * allocated from a pool created with rte_pktmbuf_pool_create(). > > + * For each packet to send, the rte_eth_tx_prepare() function performs > > + * the following operations: > > + * > > + * - Check if packet meets devices requirements for tx offloads. > > + * > > + * - Check limitations about number of segments. > > + * > > + * - Check additional requirements when debug is enabled. > > + * > > + * - Update and/or reset required checksums when tx offload is set for > > packet. > > + * > > + * Since this function can modify packet data, provided mbufs must be > > safely > > + * writable (e.g. modified data cannot be in shared segment). > > I think we will have to remove this limitation in next releases. > As we don't know how it could affect the API, I suggest to declare this > API EXPERIMENTAL. While I don't really mind to mart it as experimental, I don't really understand the reasoning: Why " this function can modify packet data, provided mbufs must be safely writable" suddenly becomes a problem? That seems like and obvious limitation to me and let say tx_burst() has the same one. Second, I don't see how you are going to remove it without introducing a heavy performance impact. Konstantin
Re: [dpdk-dev] [PATCH v12 1/6] ethdev: add Tx preparation
>
> Hi Thomas,
>
> > -Original Message-
> > From: Thomas Monjalon [mailto:thomas.monja...@6wind.com]
> > Sent: Thursday, December 1, 2016 20:52
> > To: Kulasek, TomaszX
> > Cc: dev@dpdk.org; Ananyev, Konstantin ;
> > olivier.m...@6wind.com; Richardson, Bruce
> > Subject: Re: [dpdk-dev] [PATCH v12 1/6] ethdev: add Tx preparation
> >
> > 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=1024
> > > > > > CONFIG_RTE_LIBRTE_IEEE1588=n
> > > > > > CONFIG_RTE_ETHDEV_QUEUE_STAT_CNTRS=16
> > > > > > CONFIG_RTE_ETHDEV_RXTX_CALLBACKS=y
> > > > > > +CONFIG_RTE_ETHDEV_TX_PREPARE=y
> > > > >
> > > > > 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 == 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).
>
> 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.
As Tomasz pointed out the RTE_ETHDEV_TX_PREPARE was introduced to fulfill Jerin
request.
>From here:
"Low-end ARMv7,ARMv8 targets may not have PCIE-RC support and it may have
only integrated NIC controller. On those targets/configs, where integrated NIC
controller does not use tx_prep service it can made it as NOOP to save
cycles on following "rte_eth_tx_prep" and associated "if (unlikely(nb_prep
< nb_rx))" checks in the application."
According to the measurements he done it can save ~7% on some low-end ARM
machine.
You can read whole story here:
http://dpdk.org/dev/patchwork/patch/15770/
Though, if now you guys believe that this is not good enough reason,
I have absolutely no problem to remove the RTE_ETHDEV_TX_PREPARE and associated
logic.
I personally don't use ARM boxes and don't plan to,
and in theory users can still do conditional compilation at the upper layer, if
they want to.
Konstantin
Re: [dpdk-dev] [PATCH v12 1/6] ethdev: add Tx preparation
2016-12-01 22:31, Kulasek, TomaszX:
> From: Thomas Monjalon [mailto:thomas.monja...@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=1024
> > > > > > CONFIG_RTE_LIBRTE_IEEE1588=n
> > > > > > CONFIG_RTE_ETHDEV_QUEUE_STAT_CNTRS=16
> > > > > > CONFIG_RTE_ETHDEV_RXTX_CALLBACKS=y
> > > > > > +CONFIG_RTE_ETHDEV_TX_PREPARE=y
> > > > >
> > > > > 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 == 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).
How may he know? There is no comment in the config file, no documentation.
> 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.
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.
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?
Re: [dpdk-dev] [PATCH v12 1/6] ethdev: add Tx preparation
Hi Thomas,
> -Original Message-
> From: Thomas Monjalon [mailto:thomas.monja...@6wind.com]
> Sent: Thursday, December 1, 2016 20:52
> To: Kulasek, TomaszX
> Cc: dev@dpdk.org; Ananyev, Konstantin ;
> olivier.m...@6wind.com; Richardson, Bruce
> Subject: Re: [dpdk-dev] [PATCH v12 1/6] ethdev: add Tx preparation
>
> 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=1024
> > > > > CONFIG_RTE_LIBRTE_IEEE1588=n
> > > > > CONFIG_RTE_ETHDEV_QUEUE_STAT_CNTRS=16
> > > > > CONFIG_RTE_ETHDEV_RXTX_CALLBACKS=y
> > > > > +CONFIG_RTE_ETHDEV_TX_PREPARE=y
> > > >
> > > > 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 == 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).
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.
Tomasz
Re: [dpdk-dev] [PATCH v12 1/6] ethdev: add Tx preparation
On Thu, Dec 01, 2016 at 08:52:22PM +0100, Thomas Monjalon wrote:
> 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=1024
> > > > > CONFIG_RTE_LIBRTE_IEEE1588=n
> > > > > CONFIG_RTE_ETHDEV_QUEUE_STAT_CNTRS=16
> > > > > CONFIG_RTE_ETHDEV_RXTX_CALLBACKS=y
> > > > > +CONFIG_RTE_ETHDEV_TX_PREPARE=y
> > > >
> > > > 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 == 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.
The use case I was referring was "integrated NIC" case where
- DPDK target with no external NW PCI card support
AND
- The "integrated NIC" does not need tx_prepare
Re: [dpdk-dev] [PATCH v12 1/6] ethdev: add Tx preparation
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=1024
> > > > CONFIG_RTE_LIBRTE_IEEE1588=n
> > > > CONFIG_RTE_ETHDEV_QUEUE_STAT_CNTRS=16
> > > > CONFIG_RTE_ETHDEV_RXTX_CALLBACKS=y
> > > > +CONFIG_RTE_ETHDEV_TX_PREPARE=y
> > >
> > > 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 == 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.
Re: [dpdk-dev] [PATCH v12 1/6] ethdev: add Tx preparation
Hi Thomas,
Sorry, I have answered for this question in another thread and I missed about
this one. Detailed answer is below.
> -Original Message-
> From: Thomas Monjalon [mailto:thomas.monja...@6wind.com]
> Sent: Thursday, December 1, 2016 17:24
> To: Kulasek, TomaszX
> Cc: dev@dpdk.org; Ananyev, Konstantin ;
> olivier.m...@6wind.com; Richardson, Bruce
> Subject: Re: [dpdk-dev] [PATCH v12 1/6] ethdev: add Tx preparation
>
> Please, a reply to this question would be greatly appreciated.
>
> 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=1024
> > > CONFIG_RTE_LIBRTE_IEEE1588=n
> > > CONFIG_RTE_ETHDEV_QUEUE_STAT_CNTRS=16
> > > CONFIG_RTE_ETHDEV_RXTX_CALLBACKS=y
> > > +CONFIG_RTE_ETHDEV_TX_PREPARE=y
> >
> > 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 == NULL).
Tomasz
Re: [dpdk-dev] [PATCH v12 1/6] ethdev: add Tx preparation
2016-11-23 18:36, Tomasz Kulasek: > +/** > + * Process a burst of output packets on a transmit queue of an Ethernet > device. > + * > + * The rte_eth_tx_prepare() function is invoked to prepare output packets to > be > + * transmitted on the output queue *queue_id* of the Ethernet device > designated > + * by its *port_id*. > + * The *nb_pkts* parameter is the number of packets to be prepared which are > + * supplied in the *tx_pkts* array of *rte_mbuf* structures, each of them > + * allocated from a pool created with rte_pktmbuf_pool_create(). > + * For each packet to send, the rte_eth_tx_prepare() function performs > + * the following operations: > + * > + * - Check if packet meets devices requirements for tx offloads. > + * > + * - Check limitations about number of segments. > + * > + * - Check additional requirements when debug is enabled. > + * > + * - Update and/or reset required checksums when tx offload is set for > packet. > + * > + * Since this function can modify packet data, provided mbufs must be safely > + * writable (e.g. modified data cannot be in shared segment). I think we will have to remove this limitation in next releases. As we don't know how it could affect the API, I suggest to declare this API EXPERIMENTAL.
Re: [dpdk-dev] [PATCH v12 1/6] ethdev: add Tx preparation
2016-11-23 18:36, Tomasz Kulasek: > Added fields to the `struct rte_eth_desc_lim`: > > uint16_t nb_seg_max; > /**< Max number of segments per whole packet. */ > > uint16_t nb_mtu_seg_max; > /**< Max number of segments per one MTU */ How (and when) an application is supposed to use these fields? Is it useful to expose them if we make tx_prepare() mandatory?
Re: [dpdk-dev] [PATCH v12 1/6] ethdev: add Tx preparation
Please, a reply to this question would be greatly appreciated.
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=1024
> > CONFIG_RTE_LIBRTE_IEEE1588=n
> > CONFIG_RTE_ETHDEV_QUEUE_STAT_CNTRS=16
> > CONFIG_RTE_ETHDEV_RXTX_CALLBACKS=y
> > +CONFIG_RTE_ETHDEV_TX_PREPARE=y
>
> 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.
[dpdk-dev] [PATCH v12 1/6] ethdev: add Tx preparation
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=1024
> CONFIG_RTE_LIBRTE_IEEE1588=n
> CONFIG_RTE_ETHDEV_QUEUE_STAT_CNTRS=16
> CONFIG_RTE_ETHDEV_RXTX_CALLBACKS=y
> +CONFIG_RTE_ETHDEV_TX_PREPARE=y
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.
[dpdk-dev] [PATCH v12 1/6] ethdev: add Tx preparation
Added API for `rte_eth_tx_prepare`
uint16_t rte_eth_tx_prepare(uint8_t port_id, uint16_t queue_id,
struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
Added fields to the `struct rte_eth_desc_lim`:
uint16_t nb_seg_max;
/**< Max number of segments per whole packet. */
uint16_t nb_mtu_seg_max;
/**< Max number of segments per one MTU */
Added functions:
int rte_validate_tx_offload(struct rte_mbuf *m)
to validate general requirements for tx offload set in mbuf of packet
such a flag completness. In current implementation this function is
called optionaly when RTE_LIBRTE_ETHDEV_DEBUG is enabled.
int rte_net_intel_cksum_prepare(struct rte_mbuf *m)
to fix pseudo header checksum for TSO and non-TSO tcp/udp packets
before hardware tx checksum offload.
- for non-TSO tcp/udp packets full pseudo-header checksum is
counted and set.
- for TSO the IP payload length is not included.
PERFORMANCE TESTS
-
This feature was tested with modified csum engine from test-pmd.
The packet checksum preparation was moved from application to Tx
preparation step placed before burst.
We may expect some overhead costs caused by:
1) using additional callback before burst,
2) rescanning burst,
3) additional condition checking (packet validation),
4) worse optimization (e.g. packet data access, etc.)
We tested it using ixgbe Tx preparation implementation with some parts
disabled to have comparable information about the impact of different
parts of implementation.
IMPACT:
1) For unimplemented Tx preparation callback the performance impact is
negligible,
2) For packet condition check without checksum modifications (nb_segs,
available offloads, etc.) is 14626628/14252168 (~2.62% drop),
3) Full support in ixgbe driver (point 2 + packet checksum
initialization) is 14060924/13588094 (~3.48% drop)
Signed-off-by: Tomasz Kulasek
Acked-by: Konstantin Ananyev
Acked-by: Olivier Matz
---
config/common_base|1 +
lib/librte_ether/rte_ethdev.h | 106 +
lib/librte_mbuf/rte_mbuf.h| 64 +
lib/librte_net/rte_net.h | 85 +
4 files changed, 256 insertions(+)
diff --git a/config/common_base b/config/common_base
index 4bff83a..d609a88 100644
--- a/config/common_base
+++ b/config/common_base
@@ -120,6 +120,7 @@ CONFIG_RTE_MAX_QUEUES_PER_PORT=1024
CONFIG_RTE_LIBRTE_IEEE1588=n
CONFIG_RTE_ETHDEV_QUEUE_STAT_CNTRS=16
CONFIG_RTE_ETHDEV_RXTX_CALLBACKS=y
+CONFIG_RTE_ETHDEV_TX_PREPARE=y
#
# Support NIC bypass logic
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 9678179..4ffc1b3 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -182,6 +182,7 @@
#include
#include
#include
+#include
#include "rte_ether.h"
#include "rte_eth_ctrl.h"
#include "rte_dev_info.h"
@@ -702,6 +703,8 @@ struct rte_eth_desc_lim {
uint16_t nb_max; /**< Max allowed number of descriptors. */
uint16_t nb_min; /**< Min allowed number of descriptors. */
uint16_t nb_align; /**< Number of descriptors should be aligned to. */
+ uint16_t nb_seg_max; /**< Max number of segments per whole packet.
*/
+ uint16_t nb_mtu_seg_max; /**< Max number of segments per one MTU */
};
/**
@@ -1191,6 +1194,11 @@ typedef uint16_t (*eth_tx_burst_t)(void *txq,
uint16_t nb_pkts);
/**< @internal Send output packets on a transmit queue of an Ethernet device.
*/
+typedef uint16_t (*eth_tx_prep_t)(void *txq,
+ struct rte_mbuf **tx_pkts,
+ uint16_t nb_pkts);
+/**< @internal Prepare output packets on a transmit queue of an Ethernet
device. */
+
typedef int (*flow_ctrl_get_t)(struct rte_eth_dev *dev,
struct rte_eth_fc_conf *fc_conf);
/**< @internal Get current flow control parameter on an Ethernet device */
@@ -1625,6 +1633,7 @@ struct rte_eth_rxtx_callback {
struct rte_eth_dev {
eth_rx_burst_t rx_pkt_burst; /**< Pointer to PMD receive function. */
eth_tx_burst_t tx_pkt_burst; /**< Pointer to PMD transmit function. */
+ eth_tx_prep_t tx_pkt_prepare; /**< Pointer to PMD transmit prepare
function. */
struct rte_eth_dev_data *data; /**< Pointer to device data */
const struct eth_driver *driver;/**< Driver for this device */
const struct eth_dev_ops *dev_ops; /**< Functions exported by PMD */
@@ -2819,6 +2828,103 @@ int rte_eth_dev_set_vlan_ether_type(uint8_t port_id,
return (*dev->tx_pkt_burst)(dev->data->tx_queues[queue_id], tx_pkts,
nb_pkts);
}
+/**
+ * Process a burst of output packets on a transmit queue of an Ethernet device.
+ *
+ * The rte_eth_tx_prepare() function is invoked to prepare output packets to be
+ * transmitted on the output queue *queu
