[dpdk-dev,01/39] examples/l2fwd: convert to new ethdev offloads API
Checks
Commit Message
Ethdev offloads API has changed since:
commit ce17eddefc20 ("ethdev: introduce Rx queue offloads API")
commit cba7f53b717d ("ethdev: introduce Tx queue offloads API")
This commit support the new API.
Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
---
examples/l2fwd/main.c | 38 ++++++++++++++++++++++++++++++--------
1 file changed, 30 insertions(+), 8 deletions(-)
Comments
On 11/23/2017 03:14 PM, Shahaf Shuler wrote:
> Ethdev offloads API has changed since:
>
> commit ce17eddefc20 ("ethdev: introduce Rx queue offloads API")
> commit cba7f53b717d ("ethdev: introduce Tx queue offloads API")
>
> This commit support the new API.
>
> Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> ---
> examples/l2fwd/main.c | 38 ++++++++++++++++++++++++++++++--------
> 1 file changed, 30 insertions(+), 8 deletions(-)
>
> diff --git a/examples/l2fwd/main.c b/examples/l2fwd/main.c
> index e89e2e1bf..a1e378be6 100644
> --- a/examples/l2fwd/main.c
> +++ b/examples/l2fwd/main.c
> @@ -110,14 +110,11 @@ struct lcore_queue_conf lcore_queue_conf[RTE_MAX_LCORE];
>
> static struct rte_eth_dev_tx_buffer *tx_buffer[RTE_MAX_ETHPORTS];
>
> -static const struct rte_eth_conf port_conf = {
> +static 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 */
> - .jumbo_frame = 0, /**< Jumbo Frame Support disabled */
> - .hw_strip_crc = 1, /**< CRC stripped by hardware */
> + .ignore_offload_bitfield = 1,
> + .offloads = DEV_RX_OFFLOAD_CRC_STRIP,
It is not directly related to the patch.
May be I miss something, but it looks like there is no way to say that
"I always strip CRC and cannot preserve it".
> },
> .txmode = {
> .mq_mode = ETH_MQ_TX_NONE,
> @@ -649,6 +646,9 @@ main(int argc, char **argv)
>
> /* Initialise each port */
> for (portid = 0; portid < nb_ports; portid++) {
> + struct rte_eth_rxconf rxq_conf;
> + struct rte_eth_txconf txq_conf;
> +
> /* skip ports that are not enabled */
> if ((l2fwd_enabled_port_mask & (1 << portid)) == 0) {
> printf("Skipping disabled port %u\n", portid);
> @@ -658,6 +658,23 @@ main(int argc, char **argv)
> /* init port */
> printf("Initializing port %u... ", portid);
> fflush(stdout);
> + rte_eth_dev_info_get(portid, &dev_info);
> + if ((dev_info.rx_offload_capa & port_conf.rxmode.offloads) !=
> + port_conf.rxmode.offloads) {
> + printf("Some Rx offloads are not supported "
> + "by port %d: requested 0x%lx supported 0x%lx\n",
> + portid, port_conf.rxmode.offloads,
> + dev_info.rx_offload_capa);
> + port_conf.rxmode.offloads &= dev_info.rx_offload_capa;
> + }
> + if ((dev_info.tx_offload_capa & port_conf.txmode.offloads) !=
> + port_conf.txmode.offloads) {
> + printf("Some Tx offloads are not supported "
> + "by port %d: requested 0x%lx supported 0x%lx\n",
> + portid, port_conf.txmode.offloads,
> + dev_info.tx_offload_capa);
> + port_conf.txmode.offloads &= dev_info.tx_offload_capa;
> + }
> ret = rte_eth_dev_configure(portid, 1, 1, &port_conf);
> if (ret < 0)
> rte_exit(EXIT_FAILURE, "Cannot configure device: err=%d, port=%u\n",
> @@ -674,9 +691,11 @@ main(int argc, char **argv)
>
> /* init one RX queue */
> fflush(stdout);
> + rxq_conf = dev_info.default_rxconf;
> + rxq_conf.offloads = port_conf.rxmode.offloads;
> ret = rte_eth_rx_queue_setup(portid, 0, nb_rxd,
> rte_eth_dev_socket_id(portid),
> - NULL,
> + &rxq_conf,
> l2fwd_pktmbuf_pool);
> if (ret < 0)
> rte_exit(EXIT_FAILURE, "rte_eth_rx_queue_setup:err=%d, port=%u\n",
> @@ -684,9 +703,12 @@ main(int argc, char **argv)
>
> /* init one TX queue on each port */
> fflush(stdout);
> + txq_conf = dev_info.default_txconf;
> + txq_conf.txq_flags = ETH_TXQ_FLAGS_IGNORE;
> + txq_conf.offloads = port_conf.txmode.offloads;
It looks like it is not 100% equivalent. As far as I can see dev_info
get does
not convert txq_flags to offloads in default_txconf and in any case
txq_conf.offloads
are overwritten here. So, if PMD provides default txq_flags, it is lost.
If it is intentionally, it should be highlighted and explained.
> ret = rte_eth_tx_queue_setup(portid, 0, nb_txd,
> rte_eth_dev_socket_id(portid),
> - NULL);
> + &txq_conf);
> if (ret < 0)
> rte_exit(EXIT_FAILURE, "rte_eth_tx_queue_setup:err=%d, port=%u\n",
> ret, portid);
>>+ .ignore_offload_bitfield = 1,
>>+ .offloads = DEV_RX_OFFLOAD_CRC_STRIP,
>
>It is not directly related to the patch.
>May be I miss something, but it looks like there is no way to say that
>"I always strip CRC and cannot preserve it".
Yes this is right. Not exposing the CRC offload flag means the device don’t support CRC strip toggling, however it does not explicitly say if device always strip/not.
I guess device that has such limitation should specify it on the “Limitation” section of the PMD guide.
>>+ txq_conf = dev_info.default_txconf;
>>+ txq_conf.txq_flags = ETH_TXQ_FLAGS_IGNORE;
>>+ txq_conf.offloads = port_conf.txmode.offloads;
>
>It looks like it is not 100% equivalent. As far as I can see dev_info get does
>not convert txq_flags to offloads in default_txconf and in any case txq_conf.offloads
>are overwritten here. So, if PMD provides default txq_flags, it is lost.
>If it is intentionally, it should be highlighted and explained.
Yes it is.
With the new Tx offloads API the application can choose the Tx offloads it wants to use according to its needs.
For l2fwd case – it doesn’t use any of them. Any default txq flag the PMD set there is irrelevant.
What I tried to do is not to preserve the entire old behavior rather to evolve the examples/applications while keeping the same functionality (i.e. the offloads which the application use are set, the rest are not).
Moreover – it is a wrong approach, IMO, that the PMD set default offloads flags to the application. It has no knowledge to do so. I think this mechanism was initially created since the Tx offloads were all set by default, so it provided a mean to have good OOB configuration. Now when all offloads are set, I am not sure such API is needed anymore.
Will be happy to hear more opinion on that.
--Shahaf
On 11/26/2017 10:41 AM, Shahaf Shuler wrote:
> >>+ .ignore_offload_bitfield = 1,
> >>+ .offloads = DEV_RX_OFFLOAD_CRC_STRIP,
>
> >
> >It is not directly related to the patch.
> >May be I miss something, but it looks like there is no way to say that
> >"I always strip CRC and cannot preserve it".
>
> Yes this is right. Not exposing the CRC offload flag means the device
> don’t support CRC strip toggling, however it does not explicitly say
> if device always strip/not.
>
> I guess device that has such limitation should specify it on the
> “Limitation” section of the PMD guide.
>
If it is interpreted in such way it sounds like loss of functionality.
Don't think it is a good way to rely on documentation here. It should
be more reliable way. PMD still can check if offload is not enabled and
complain, but there is no way to say that it is strictly required.
As I understand similar things are covered with so-called fixed offloads
in Linux.
> >>+ txq_conf = dev_info.default_txconf;
> >>+ txq_conf.txq_flags = ETH_TXQ_FLAGS_IGNORE;
> >>+ txq_conf.offloads = port_conf.txmode.offloads;
>
> >
> >It looks like it is not 100% equivalent. As far as I can see dev_info
> get does
> >not convert txq_flags to offloads in default_txconf and in any case
> txq_conf.offloads
> >are overwritten here. So, if PMD provides default txq_flags, it is lost.
> >If it is intentionally, it should be highlighted and explained.
>
> Yes it is.
>
> With the new Tx offloads API the application can choose the Tx
> offloads it wants to use according to its needs.
>
> For l2fwd case – it doesn’t use any of them. Any default txq flag the
> PMD set there is irrelevant.
>
> What I tried to do is not to preserve the entire old behavior rather
> to evolve the examples/applications while keeping the same
> functionality (i.e. the offloads which the application use are set,
> the rest are not).
>
That's true for checksum and VLAN offloads, but false for fast-free.
As I understand l2fwd and many other examples meet fast-free
requirements and if PMD supports it, it should be used since it will
show better performance results.
> Moreover – it is a wrong approach, IMO, that the PMD set default
> offloads flags to the application. It has no knowledge to do so. I
> think this mechanism was initially created since the Tx offloads were
> all set by default, so it provided a mean to have good OOB
> configuration. Now when all offloads are set, I am not sure such API
> is needed anymore.
>
> Will be happy to hear more opinion on that.
>
I agree that blindly using PMD default offloads is a wrong approach.
Monday, November 27, 2017 8:35 AM, Andrew Rybchenko:
Yes this is right. Not exposing the CRC offload flag means the device don’t support CRC strip toggling, however it does not explicitly say if device always strip/not.
I guess device that has such limitation should specify it on the “Limitation” section of the PMD guide.
If it is interpreted in such way it sounds like loss of functionality.
Don't think it is a good way to rely on documentation here. It should
be more reliable way. PMD still can check if offload is not enabled and
complain, but there is no way to say that it is strictly required.
As I understand similar things are covered with so-called fixed offloads
in Linux.
Can you elaborate which functionality is being lost here?
If your suggestion is for the PMD to force the CRC STRIP offload in case it is not supporting *not* to strip CRC then I am OK with that.
Yes it is.
With the new Tx offloads API the application can choose the Tx offloads it wants to use according to its needs.
For l2fwd case – it doesn’t use any of them. Any default txq flag the PMD set there is irrelevant.
What I tried to do is not to preserve the entire old behavior rather to evolve the examples/applications while keeping the same functionality (i.e. the offloads which the application use are set, the rest are not).
That's true for checksum and VLAN offloads, but false for fast-free.
As I understand l2fwd and many other examples meet fast-free
requirements and if PMD supports it, it should be used since it will
show better performance results.
I agree about the Fast free offload. However IMO such optimization can be introduced on other series which further more optimize the performance of such applications, what do you think?
--Shahaf
From: Andrew Rybchenko [mailto:arybchenko@solarflare.com]
Sent: Monday, November 27, 2017 8:35 AM
To: Shahaf Shuler <shahafs@mellanox.com>; dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH 01/39] examples/l2fwd: convert to new ethdev offloads API
On 11/26/2017 10:41 AM, Shahaf Shuler wrote:
>>+ .ignore_offload_bitfield = 1,
>>+ .offloads = DEV_RX_OFFLOAD_CRC_STRIP,
>
>It is not directly related to the patch.
>May be I miss something, but it looks like there is no way to say that
>"I always strip CRC and cannot preserve it".
Yes this is right. Not exposing the CRC offload flag means the device don’t support CRC strip toggling, however it does not explicitly say if device always strip/not.
I guess device that has such limitation should specify it on the “Limitation” section of the PMD guide.
If it is interpreted in such way it sounds like loss of functionality.
Don't think it is a good way to rely on documentation here. It should
be more reliable way. PMD still can check if offload is not enabled and
complain, but there is no way to say that it is strictly required.
As I understand similar things are covered with so-called fixed offloads
in Linux.
>>+ txq_conf = dev_info.default_txconf;
>>+ txq_conf.txq_flags = ETH_TXQ_FLAGS_IGNORE;
>>+ txq_conf.offloads = port_conf.txmode.offloads;
>
>It looks like it is not 100% equivalent. As far as I can see dev_info get does
>not convert txq_flags to offloads in default_txconf and in any case txq_conf.offloads
>are overwritten here. So, if PMD provides default txq_flags, it is lost.
>If it is intentionally, it should be highlighted and explained.
Yes it is.
With the new Tx offloads API the application can choose the Tx offloads it wants to use according to its needs.
For l2fwd case – it doesn’t use any of them. Any default txq flag the PMD set there is irrelevant.
What I tried to do is not to preserve the entire old behavior rather to evolve the examples/applications while keeping the same functionality (i.e. the offloads which the application use are set, the rest are not).
That's true for checksum and VLAN offloads, but false for fast-free.
As I understand l2fwd and many other examples meet fast-free
requirements and if PMD supports it, it should be used since it will
show better performance results.
Moreover – it is a wrong approach, IMO, that the PMD set default offloads flags to the application. It has no knowledge to do so. I think this mechanism was initially created since the Tx offloads were all set by default, so it provided a mean to have good OOB configuration. Now when all offloads are set, I am not sure such API is needed anymore.
Will be happy to hear more opinion on that.
I agree that blindly using PMD default offloads is a wrong approach.
-----Original Message-----
> Date: Mon, 27 Nov 2017 07:03:54 +0000
> From: Shahaf Shuler <shahafs@mellanox.com>
> To: Andrew Rybchenko <arybchenko@solarflare.com>, "dev@dpdk.org"
> <dev@dpdk.org>
> Subject: Re: [dpdk-dev] [PATCH 01/39] examples/l2fwd: convert to new ethdev
> offloads API
>
> Monday, November 27, 2017 8:35 AM, Andrew Rybchenko:
>
> Yes this is right. Not exposing the CRC offload flag means the device don’t support CRC strip toggling, however it does not explicitly say if device always strip/not.
> I guess device that has such limitation should specify it on the “Limitation” section of the PMD guide.
>
> If it is interpreted in such way it sounds like loss of functionality.
> Don't think it is a good way to rely on documentation here. It should
> be more reliable way. PMD still can check if offload is not enabled and
> complain, but there is no way to say that it is strictly required.
> As I understand similar things are covered with so-called fixed offloads
> in Linux.
>
> Can you elaborate which functionality is being lost here?
> If your suggestion is for the PMD to force the CRC STRIP offload in case it is not supporting *not* to strip CRC then I am OK with that.
>
> Yes it is.
> With the new Tx offloads API the application can choose the Tx offloads it wants to use according to its needs.
> For l2fwd case – it doesn’t use any of them. Any default txq flag the PMD set there is irrelevant.
> What I tried to do is not to preserve the entire old behavior rather to evolve the examples/applications while keeping the same functionality (i.e. the offloads which the application use are set, the rest are not).
>
> That's true for checksum and VLAN offloads, but false for fast-free.
> As I understand l2fwd and many other examples meet fast-free
> requirements and if PMD supports it, it should be used since it will
> show better performance results.
>
> I agree about the Fast free offload. However IMO such optimization can be introduced on other series which further more optimize the performance of such applications, what do you think?
If Fast free offload is meeting the l2fwd or l3fwd example application requirements, Why not
to add it now?
>
> --Shahaf
>
> From: Andrew Rybchenko [mailto:arybchenko@solarflare.com]
> Sent: Monday, November 27, 2017 8:35 AM
> To: Shahaf Shuler <shahafs@mellanox.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 01/39] examples/l2fwd: convert to new ethdev offloads API
>
> On 11/26/2017 10:41 AM, Shahaf Shuler wrote:
>
> >>+ .ignore_offload_bitfield = 1,
>
> >>+ .offloads = DEV_RX_OFFLOAD_CRC_STRIP,
> >
> >It is not directly related to the patch.
> >May be I miss something, but it looks like there is no way to say that
> >"I always strip CRC and cannot preserve it".
>
> Yes this is right. Not exposing the CRC offload flag means the device don’t support CRC strip toggling, however it does not explicitly say if device always strip/not.
> I guess device that has such limitation should specify it on the “Limitation” section of the PMD guide.
>
> If it is interpreted in such way it sounds like loss of functionality.
> Don't think it is a good way to rely on documentation here. It should
> be more reliable way. PMD still can check if offload is not enabled and
> complain, but there is no way to say that it is strictly required.
> As I understand similar things are covered with so-called fixed offloads
> in Linux.
>
>
>
> >>+ txq_conf = dev_info.default_txconf;
>
> >>+ txq_conf.txq_flags = ETH_TXQ_FLAGS_IGNORE;
>
> >>+ txq_conf.offloads = port_conf.txmode.offloads;
> >
> >It looks like it is not 100% equivalent. As far as I can see dev_info get does
> >not convert txq_flags to offloads in default_txconf and in any case txq_conf.offloads
> >are overwritten here. So, if PMD provides default txq_flags, it is lost.
> >If it is intentionally, it should be highlighted and explained.
>
> Yes it is.
> With the new Tx offloads API the application can choose the Tx offloads it wants to use according to its needs.
> For l2fwd case – it doesn’t use any of them. Any default txq flag the PMD set there is irrelevant.
> What I tried to do is not to preserve the entire old behavior rather to evolve the examples/applications while keeping the same functionality (i.e. the offloads which the application use are set, the rest are not).
>
> That's true for checksum and VLAN offloads, but false for fast-free.
> As I understand l2fwd and many other examples meet fast-free
> requirements and if PMD supports it, it should be used since it will
> show better performance results.
>
>
> Moreover – it is a wrong approach, IMO, that the PMD set default offloads flags to the application. It has no knowledge to do so. I think this mechanism was initially created since the Tx offloads were all set by default, so it provided a mean to have good OOB configuration. Now when all offloads are set, I am not sure such API is needed anymore.
> Will be happy to hear more opinion on that.
>
> I agree that blindly using PMD default offloads is a wrong approach.
On 11/27/2017 10:03 AM, Shahaf Shuler wrote:
>
> Monday, November 27, 2017 8:35 AM, Andrew Rybchenko:
>
> Yes this is right. Not exposing the CRC offload flag means the device
> don’t support CRC strip toggling, however it does not explicitly say
> if device always strip/not.
>
> I guess device that has such limitation should specify it on the
> “Limitation” section of the PMD guide.
>
> If it is interpreted in such way it sounds like loss of functionality.
> Don't think it is a good way to rely on documentation here. It should
> be more reliable way. PMD still can check if offload is not enabled and
> complain, but there is no way to say that it is strictly required.
> As I understand similar things are covered with so-called fixed offloads
> in Linux.
>
> Can you elaborate which functionality is being lost here?
>
> If your suggestion is for the PMD to force the CRC STRIP offload in
> case it is not supporting **not** to strip CRC then I am OK with that.
>
[AR] I mean that if we say that no CRC strip offload set means either
strip or not-strip it is bad. Application and transmit part may rely on
Ethernet CRC presence. My suggestion is to provide a way to say that CRC
striping (and any other offload) cannot be disabled.
>
> Yes it is.
>
> With the new Tx offloads API the application can choose the Tx
> offloads it wants to use according to its needs.
>
> For l2fwd case – it doesn’t use any of them. Any default txq flag the
> PMD set there is irrelevant.
>
> What I tried to do is not to preserve the entire old behavior rather
> to evolve the examples/applications while keeping the same
> functionality (i.e. the offloads which the application use are set,
> the rest are not).
>
>
> That's true for checksum and VLAN offloads, but false for fast-free.
> As I understand l2fwd and many other examples meet fast-free
> requirements and if PMD supports it, it should be used since it will
> show better performance results.
>
> I agree about the Fast free offload. However IMO such optimization can
> be introduced on other series which further more optimize the
> performance of such applications, what do you think?
>
[AR] If so, it is definitely a loss of functionality here. Since before
the patch PMD can provide default TxQ flags and usage of these flags
will allow an application to see the best performance. (Yes, if it was
bad if application blindly use these flags). May be it should be covered
by separate patches, but hanging it in the air for a long time is bad. I
definitely would like to hear more opinions here from example
application and PMD maintainers.
Monday, November 27, 2017 9:34 AM, Jerin Jacob:
> >
> > Monday, November 27, 2017 8:35 AM, Andrew Rybchenko:
> >
> > I agree about the Fast free offload. However IMO such optimization can be
> introduced on other series which further more optimize the performance of
> such applications, what do you think?
>
> If Fast free offload is meeting the l2fwd or l3fwd example application
> requirements, Why not to add it now?
>
Andrew, Jerin,
Agreed. Fast free offload, when possible, will be part of v2.
@@ -110,14 +110,11 @@ struct lcore_queue_conf lcore_queue_conf[RTE_MAX_LCORE];
static struct rte_eth_dev_tx_buffer *tx_buffer[RTE_MAX_ETHPORTS];
-static const struct rte_eth_conf port_conf = {
+static 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 */
- .jumbo_frame = 0, /**< Jumbo Frame Support disabled */
- .hw_strip_crc = 1, /**< CRC stripped by hardware */
+ .ignore_offload_bitfield = 1,
+ .offloads = DEV_RX_OFFLOAD_CRC_STRIP,
},
.txmode = {
.mq_mode = ETH_MQ_TX_NONE,
@@ -649,6 +646,9 @@ main(int argc, char **argv)
/* Initialise each port */
for (portid = 0; portid < nb_ports; portid++) {
+ struct rte_eth_rxconf rxq_conf;
+ struct rte_eth_txconf txq_conf;
+
/* skip ports that are not enabled */
if ((l2fwd_enabled_port_mask & (1 << portid)) == 0) {
printf("Skipping disabled port %u\n", portid);
@@ -658,6 +658,23 @@ main(int argc, char **argv)
/* init port */
printf("Initializing port %u... ", portid);
fflush(stdout);
+ rte_eth_dev_info_get(portid, &dev_info);
+ if ((dev_info.rx_offload_capa & port_conf.rxmode.offloads) !=
+ port_conf.rxmode.offloads) {
+ printf("Some Rx offloads are not supported "
+ "by port %d: requested 0x%lx supported 0x%lx\n",
+ portid, port_conf.rxmode.offloads,
+ dev_info.rx_offload_capa);
+ port_conf.rxmode.offloads &= dev_info.rx_offload_capa;
+ }
+ if ((dev_info.tx_offload_capa & port_conf.txmode.offloads) !=
+ port_conf.txmode.offloads) {
+ printf("Some Tx offloads are not supported "
+ "by port %d: requested 0x%lx supported 0x%lx\n",
+ portid, port_conf.txmode.offloads,
+ dev_info.tx_offload_capa);
+ port_conf.txmode.offloads &= dev_info.tx_offload_capa;
+ }
ret = rte_eth_dev_configure(portid, 1, 1, &port_conf);
if (ret < 0)
rte_exit(EXIT_FAILURE, "Cannot configure device: err=%d, port=%u\n",
@@ -674,9 +691,11 @@ main(int argc, char **argv)
/* init one RX queue */
fflush(stdout);
+ rxq_conf = dev_info.default_rxconf;
+ rxq_conf.offloads = port_conf.rxmode.offloads;
ret = rte_eth_rx_queue_setup(portid, 0, nb_rxd,
rte_eth_dev_socket_id(portid),
- NULL,
+ &rxq_conf,
l2fwd_pktmbuf_pool);
if (ret < 0)
rte_exit(EXIT_FAILURE, "rte_eth_rx_queue_setup:err=%d, port=%u\n",
@@ -684,9 +703,12 @@ main(int argc, char **argv)
/* init one TX queue on each port */
fflush(stdout);
+ txq_conf = dev_info.default_txconf;
+ txq_conf.txq_flags = ETH_TXQ_FLAGS_IGNORE;
+ txq_conf.offloads = port_conf.txmode.offloads;
ret = rte_eth_tx_queue_setup(portid, 0, nb_txd,
rte_eth_dev_socket_id(portid),
- NULL);
+ &txq_conf);
if (ret < 0)
rte_exit(EXIT_FAILURE, "rte_eth_tx_queue_setup:err=%d, port=%u\n",
ret, portid);