[dpdk-dev,1/5] app/testpmd: convert to new Ethdev offloads API
Checks
Commit Message
Ethdev Rx/Tx offloads API has changed since:
commit ce17eddefc20 ("ethdev: introduce Rx queue offloads API")
commit cba7f53b717d ("ethdev: introduce Tx queue offloads API")
Convert the application to use the new API.
Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
---
app/test-pmd/cmdline.c | 148 +++++++++++++++++++++++++++++++++--------
app/test-pmd/config.c | 97 +++++++++++++++++++--------
app/test-pmd/parameters.c | 35 +++++-----
app/test-pmd/testpmd.c | 24 +++----
4 files changed, 219 insertions(+), 85 deletions(-)
Comments
On 11/23/2017 4:08 AM, Shahaf Shuler wrote:
> Ethdev Rx/Tx offloads API has changed since:
>
> commit ce17eddefc20 ("ethdev: introduce Rx queue offloads API")
> commit cba7f53b717d ("ethdev: introduce Tx queue offloads API")
>
> Convert the application to use the new API.
Hi Shahaf,
As far as I can see patch does a few things:
1- Convert rxmode bitfields usage to rxmode offloads usage.
2- Apply some config options to port config and add some port config checks.
3- Adding device advertised capability checks for some offloads.
Would you mind separate 2 and 3 to their own patches, with that 1 should be
straightforward and 2 & 3 will be easy to review.
And is this update tested with PMDs both support new offload method and old
offload method?
Thanks,
ferruh
>
> Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
<...>
> } else if (!strcmp(res->name, "hw-vlan-extend")) {
> if (!strcmp(res->value, "on"))
> - rx_mode.hw_vlan_extend = 1;
> + rx_offloads |= DEV_RX_OFFLOAD_VLAN_EXTEND;
Not related to this patch, but since you are touching these, what is the
difference between DEV_RX_OFFLOAD_VLAN_EXTEND and DEV_RX_OFFLOAD_QINQ_STRIP ?
> @@ -3434,7 +3439,14 @@ cmd_tx_vlan_set_parsed(void *parsed_result,
> {
> struct cmd_tx_vlan_set_result *res = parsed_result;
>
> + if (!all_ports_stopped()) {
> + printf("Please stop all ports first\n");
> + return;
> + }
rte_eth_rxmode bitfields to "offloads" conversion is mostly straightforward, but
is above kind of modifications part of this conversion or are you adding missing
checks?
I would prefer making only conversion related changes in this patch, and extra
improvements in other patch.
> +
> tx_vlan_set(res->port_id, res->vlan_id);
> +
> + cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
Is this required for converting bitfield to offloads usage?
> }
>
> cmdline_parse_token_string_t cmd_tx_vlan_set_tx_vlan =
> @@ -3481,7 +3493,14 @@ cmd_tx_vlan_set_qinq_parsed(void *parsed_result,
> {
> struct cmd_tx_vlan_set_qinq_result *res = parsed_result;
>
> + if (!all_ports_stopped()) {
> + printf("Please stop all ports first\n");
> + return;
> + }
Same for all occurrence of these updates.
<...>
> @@ -3693,22 +3724,34 @@ cmd_csum_parsed(void *parsed_result,
>
> if (!strcmp(res->proto, "ip")) {
> mask = TESTPMD_TX_OFFLOAD_IP_CKSUM;
> + csum_offloads |= DEV_TX_OFFLOAD_IPV4_CKSUM;
> } else if (!strcmp(res->proto, "udp")) {
> mask = TESTPMD_TX_OFFLOAD_UDP_CKSUM;
> + csum_offloads |= DEV_TX_OFFLOAD_UDP_CKSUM;
> } else if (!strcmp(res->proto, "tcp")) {
> mask = TESTPMD_TX_OFFLOAD_TCP_CKSUM;
> + csum_offloads |= DEV_TX_OFFLOAD_TCP_CKSUM;
> } else if (!strcmp(res->proto, "sctp")) {
> mask = TESTPMD_TX_OFFLOAD_SCTP_CKSUM;
> + csum_offloads |= DEV_TX_OFFLOAD_SCTP_CKSUM;
> } else if (!strcmp(res->proto, "outer-ip")) {
> mask = TESTPMD_TX_OFFLOAD_OUTER_IP_CKSUM;
> + csum_offloads |= DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM;
> }
>
> - if (hw)
> + if (hw) {
> ports[res->port_id].tx_ol_flags |= mask;
> - else
> + ports[res->port_id].dev_conf.txmode.offloads |=
> + csum_offloads;
So you are updating port config as well as testpmd internal configuration, again
I guess this is not related to conversion to offloads usage.
<...>
> @@ -13017,6 +13093,13 @@ cmd_set_macsec_offload_on_parsed(
>
> switch (ret) {
> case 0:
> + rte_eth_dev_info_get(port_id, &dev_info);
> + if ((dev_info.tx_offload_capa &
> + DEV_TX_OFFLOAD_MACSEC_INSERT) == 0) {
> + printf("Warning: macsec insert enabled but not "
> + "supported by port %d\n", port_id);
> + }
This also adding another layer of check if device advertise requested
capability, this is an improvement independent from conversion, can you please
separate into its own patch?
<...>
> @@ -606,8 +616,8 @@ port_offload_cap_display(portid_t port_id)
>
> if (dev_info.tx_offload_capa & DEV_TX_OFFLOAD_VLAN_INSERT) {
> printf("VLAN insert: ");
> - if (ports[port_id].tx_ol_flags &
> - TESTPMD_TX_OFFLOAD_INSERT_VLAN)
This is removing testpmd local config check, just to double check if all places
that updates this local config covered to update device config variable?
And do we still need testpmd tx_ol_flags after these changes?
<...>
> @@ -1658,7 +1679,8 @@ rxtx_config_display(void)
> printf(" %s packet forwarding%s - CRC stripping %s - "
> "packets/burst=%d\n", cur_fwd_eng->fwd_mode_name,
> retry_enabled == 0 ? "" : " with retry",
> - rx_mode.hw_strip_crc ? "enabled" : "disabled",
> + (ports[0].dev_conf.rxmode.offloads & DEV_RX_OFFLOAD_CRC_STRIP) ?
> + "enabled" : "disabled",
There is a global config option in testpmd, for all ports. Previous log was
print based on that config option, but now you are printing the value of first port.
I believe it is wrong to display only first port values, either log can be
updated to say testpmd default configs, or remove completely, or print for all
ports, what do you think?
<...>
> @@ -897,34 +900,31 @@ launch_args_parse(int argc, char** argv)
> }
> #endif
> if (!strcmp(lgopts[opt_idx].name, "disable-crc-strip"))
> - rx_mode.hw_strip_crc = 0;
> + rx_offloads &= ~DEV_RX_OFFLOAD_CRC_STRIP;
> if (!strcmp(lgopts[opt_idx].name, "enable-lro"))
> - rx_mode.enable_lro = 1;
> - if (!strcmp(lgopts[opt_idx].name, "enable-scatter"))
> - rx_mode.enable_scatter = 1;
> + rx_offloads |= DEV_RX_OFFLOAD_TCP_LRO;
> + if (!strcmp(lgopts[opt_idx].name, "enable-scatter")) {
> + rx_offloads |= DEV_RX_OFFLOAD_SCATTER;
> + }
Can drop "{}"
<...>
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index c3ab44849..e3a7c26b8 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -338,15 +338,10 @@ lcoreid_t latencystats_lcore_id = -1;
> */
> struct rte_eth_rxmode rx_mode = {
> .max_rx_pkt_len = ETHER_MAX_LEN, /**< Default maximum frame length. */
> - .split_hdr_size = 0,
> - .header_split = 0, /**< Header Split disabled. */
> - .hw_ip_checksum = 0, /**< IP checksum offload disabled. */
> - .hw_vlan_filter = 1, /**< VLAN filtering enabled. */
> - .hw_vlan_strip = 1, /**< VLAN strip enabled. */
> - .hw_vlan_extend = 0, /**< Extended VLAN disabled. */
> - .jumbo_frame = 0, /**< Jumbo Frame Support disabled. */
> - .hw_strip_crc = 1, /**< CRC stripping by hardware enabled. */
> - .hw_timestamp = 0, /**< HW timestamp enabled. */
> + .offloads = (DEV_RX_OFFLOAD_VLAN_FILTER |
> + DEV_RX_OFFLOAD_VLAN_STRIP |
> + DEV_RX_OFFLOAD_CRC_STRIP),
> + .ignore_offload_bitfield = 1, /**< Use rte_eth_rxq_conf offloads API */
Is comment correct?
Flag has two meaning I guess,
1) Ignore bitfield values for port based offload configuration.
2) For rxq, use rx_conf.offloads field.
testpmd is still using port based offload (rte_eth_rxmode) but "offloads"
variable instead of bitfields, right? queue specific ones are copy of port configs.
<...>
Tuesday, December 5, 2017 12:31 AM, Ferruh Yigit:
> On 11/23/2017 4:08 AM, Shahaf Shuler wrote:
> > Ethdev Rx/Tx offloads API has changed since:
> >
> > commit ce17eddefc20 ("ethdev: introduce Rx queue offloads API") commit
> > cba7f53b717d ("ethdev: introduce Tx queue offloads API")
> >
> > Convert the application to use the new API.
>
> Hi Shahaf,
>
> As far as I can see patch does a few things:
> 1- Convert rxmode bitfields usage to rxmode offloads usage.
> 2- Apply some config options to port config and add some port config checks.
> 3- Adding device advertised capability checks for some offloads.
>
> Would you mind separate 2 and 3 to their own patches, with that 1 should be
> straightforward and 2 & 3 will be easy to review.
See below comments. #2 on your list is actually needed for the conversion patch.
I can split the extra cap check if you fill it needs to be in a separate patch.
>
>
> And is this update tested with PMDs both support new offload method and
> old offload method?
It was tested with mlx5 and mlx4 PMD after the conversion to the new APIs.
I put this series early so everyone can try, test and report if something is broken.
I will try to do more testing with the old mlx PMD.
BTW - we agreed that we set DD for all PMDs to move to the new API by 18.02. I still haven't see patches for that from the rest.
>
> Thanks,
> ferruh
>
> >
> > Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
>
> <...>
>
> > } else if (!strcmp(res->name, "hw-vlan-extend")) {
> > if (!strcmp(res->value, "on"))
> > - rx_mode.hw_vlan_extend = 1;
> > + rx_offloads |= DEV_RX_OFFLOAD_VLAN_EXTEND;
>
> Not related to this patch, but since you are touching these, what is the
> difference between DEV_RX_OFFLOAD_VLAN_EXTEND and
> DEV_RX_OFFLOAD_QINQ_STRIP ?
Good question, I could not figure it out either.
I guess those are identical. In the old API the hw_vlan_extend was the offload and the DEV_RX_OFFLOAD_QINQ_STRIP was the cap.
Now that we merged them we have duplication.
From one side, using DEV_RX_OFFLOAD_VLAN_EXTEND is more intuitive for application which previously used hw_vlan_extend to set the offload,
For the other side QINQ_STRIP is more explicit with respect to what the feature does, and it is the flag which is currently being used for PMDs.
So when we will change it, I guess I am in favor of the QINQ flag.
>
> > @@ -3434,7 +3439,14 @@ cmd_tx_vlan_set_parsed(void *parsed_result,
> {
> > struct cmd_tx_vlan_set_result *res = parsed_result;
> >
> > + if (!all_ports_stopped()) {
> > + printf("Please stop all ports first\n");
> > + return;
> > + }
>
> rte_eth_rxmode bitfields to "offloads" conversion is mostly straightforward,
> but is above kind of modifications part of this conversion or are you adding
> missing checks?
It is part of the conversion and this is related to testpmd design.
Previously in the old API the tx offloads were set even after the port is started. For this specific example the vlan insert just changed a flag (TESTPMD_TX_OFFLOAD_INSERT_VLAN) on the application port struct to use PKT_TX_VLAN_PKT in the mbuf ol_flags. The application didn't update the PMD in anyway. In fact, it was possible to put txq_flag which says no vlan insertion and then to set vlan insertion through the CLI.
This was OK back then because all of the Tx offloads were set by default and we assumed users know what they are doing when setting the txq_flags.
Now all the tx offloads are disabled by default. Every Tx offload being used should update the PMD (as it may need to switch tx burst function). This is why the Tx offloads configuration must be done when the port is stopped, and be followed with reconfiguration of the device and the queues.
>
> I would prefer making only conversion related changes in this patch, and
> extra improvements in other patch.
>
> > +
> > tx_vlan_set(res->port_id, res->vlan_id);
> > +
> > + cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
>
> Is this required for converting bitfield to offloads usage?
Yes, see above.
>
> > }
> >
> > cmdline_parse_token_string_t cmd_tx_vlan_set_tx_vlan = @@ -3481,7
> > +3493,14 @@ cmd_tx_vlan_set_qinq_parsed(void *parsed_result, {
> > struct cmd_tx_vlan_set_qinq_result *res = parsed_result;
> >
> > + if (!all_ports_stopped()) {
> > + printf("Please stop all ports first\n");
> > + return;
> > + }
>
> Same for all occurrence of these updates.
>
> <...>
>
> > @@ -3693,22 +3724,34 @@ cmd_csum_parsed(void *parsed_result,
> >
> > if (!strcmp(res->proto, "ip")) {
> > mask = TESTPMD_TX_OFFLOAD_IP_CKSUM;
> > + csum_offloads |= DEV_TX_OFFLOAD_IPV4_CKSUM;
> > } else if (!strcmp(res->proto, "udp")) {
> > mask = TESTPMD_TX_OFFLOAD_UDP_CKSUM;
> > + csum_offloads |= DEV_TX_OFFLOAD_UDP_CKSUM;
> > } else if (!strcmp(res->proto, "tcp")) {
> > mask = TESTPMD_TX_OFFLOAD_TCP_CKSUM;
> > + csum_offloads |= DEV_TX_OFFLOAD_TCP_CKSUM;
> > } else if (!strcmp(res->proto, "sctp")) {
> > mask = TESTPMD_TX_OFFLOAD_SCTP_CKSUM;
> > + csum_offloads |= DEV_TX_OFFLOAD_SCTP_CKSUM;
> > } else if (!strcmp(res->proto, "outer-ip")) {
> > mask = TESTPMD_TX_OFFLOAD_OUTER_IP_CKSUM;
> > + csum_offloads |=
> DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM;
> > }
> >
> > - if (hw)
> > + if (hw) {
> > ports[res->port_id].tx_ol_flags |= mask;
> > - else
> > + ports[res->port_id].dev_conf.txmode.offloads |=
> > + csum_offloads;
>
> So you are updating port config as well as testpmd internal configuration,
> again I guess this is not related to conversion to offloads usage.
It is. See above.
The port config will be used for the reconfiguration of the device and queues. This is a must for the Tx offloads .
>
> <...>
>
> > @@ -13017,6 +13093,13 @@ cmd_set_macsec_offload_on_parsed(
> >
> > switch (ret) {
> > case 0:
> > + rte_eth_dev_info_get(port_id, &dev_info);
> > + if ((dev_info.tx_offload_capa &
> > + DEV_TX_OFFLOAD_MACSEC_INSERT) == 0) {
> > + printf("Warning: macsec insert enabled but not "
> > + "supported by port %d\n", port_id);
> > + }
>
> This also adding another layer of check if device advertise requested
> capability, this is an improvement independent from conversion, can you
> please separate into its own patch?
Yes we can do it.
>
> <...>
>
> > @@ -606,8 +616,8 @@ port_offload_cap_display(portid_t port_id)
> >
> > if (dev_info.tx_offload_capa & DEV_TX_OFFLOAD_VLAN_INSERT) {
> > printf("VLAN insert: ");
> > - if (ports[port_id].tx_ol_flags &
> > - TESTPMD_TX_OFFLOAD_INSERT_VLAN)
>
> This is removing testpmd local config check, just to double check if all places
> that updates this local config covered to update device config variable?
I hope so. If you find something I missed let me know :).
>
> And do we still need testpmd tx_ol_flags after these changes?
Currently those flags are being used. One can prepare another patch to remove those and use the port config flags instead.
This is not related to the conversion and could be a nice cleanup.
>
> <...>
>
> > @@ -1658,7 +1679,8 @@ rxtx_config_display(void)
> > printf(" %s packet forwarding%s - CRC stripping %s - "
> > "packets/burst=%d\n", cur_fwd_eng->fwd_mode_name,
> > retry_enabled == 0 ? "" : " with retry",
> > - rx_mode.hw_strip_crc ? "enabled" : "disabled",
> > + (ports[0].dev_conf.rxmode.offloads &
> DEV_RX_OFFLOAD_CRC_STRIP) ?
> > + "enabled" : "disabled",
>
> There is a global config option in testpmd, for all ports. Previous log was print
> based on that config option, but now you are printing the value of first port.
Not exactly (there are multiple wrong issues with this function). For example the next lines are:
if (cur_fwd_eng == &tx_only_engine || cur_fwd_eng == &flow_gen_engine)
printf(" packet len=%u - nb packet segments=%d\n",
(unsigned)tx_pkt_length, (int) tx_pkt_nb_segs);
struct rte_eth_rxconf *rx_conf = &ports[0].rx_conf;
struct rte_eth_txconf *tx_conf = &ports[0].tx_conf;
the last were added by commit f2c5125a686a ("app/testpmd: use default Rx/Tx port configuration").
As you can see port[0] is the one being used for the rest of the configuration print.
>
> I believe it is wrong to display only first port values, either log can be updated
> to say testpmd default configs, or remove completely, or print for all ports,
> what do you think?
IMO it is the best to print for all ports.
>
> <...>
>
> > @@ -897,34 +900,31 @@ launch_args_parse(int argc, char** argv)
> > }
> > #endif
> > if (!strcmp(lgopts[opt_idx].name, "disable-crc-
> strip"))
> > - rx_mode.hw_strip_crc = 0;
> > + rx_offloads &=
> ~DEV_RX_OFFLOAD_CRC_STRIP;
> > if (!strcmp(lgopts[opt_idx].name, "enable-lro"))
> > - rx_mode.enable_lro = 1;
> > - if (!strcmp(lgopts[opt_idx].name, "enable-scatter"))
> > - rx_mode.enable_scatter = 1;
> > + rx_offloads |= DEV_RX_OFFLOAD_TCP_LRO;
> > + if (!strcmp(lgopts[opt_idx].name, "enable-scatter"))
> {
> > + rx_offloads |= DEV_RX_OFFLOAD_SCATTER;
> > + }
>
> Can drop "{}"
Yes.
>
> <...>
>
> > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> > c3ab44849..e3a7c26b8 100644
> > --- a/app/test-pmd/testpmd.c
> > +++ b/app/test-pmd/testpmd.c
> > @@ -338,15 +338,10 @@ lcoreid_t latencystats_lcore_id = -1;
> > */
> > struct rte_eth_rxmode rx_mode = {
> > .max_rx_pkt_len = ETHER_MAX_LEN, /**< Default maximum frame
> length. */
> > - .split_hdr_size = 0,
> > - .header_split = 0, /**< Header Split disabled. */
> > - .hw_ip_checksum = 0, /**< IP checksum offload disabled. */
> > - .hw_vlan_filter = 1, /**< VLAN filtering enabled. */
> > - .hw_vlan_strip = 1, /**< VLAN strip enabled. */
> > - .hw_vlan_extend = 0, /**< Extended VLAN disabled. */
> > - .jumbo_frame = 0, /**< Jumbo Frame Support disabled. */
> > - .hw_strip_crc = 1, /**< CRC stripping by hardware enabled. */
> > - .hw_timestamp = 0, /**< HW timestamp enabled. */
> > + .offloads = (DEV_RX_OFFLOAD_VLAN_FILTER |
> > + DEV_RX_OFFLOAD_VLAN_STRIP |
> > + DEV_RX_OFFLOAD_CRC_STRIP),
> > + .ignore_offload_bitfield = 1, /**< Use rte_eth_rxq_conf offloads API
> > +*/
>
> Is comment correct?
No I should remove it.
> Flag has two meaning I guess,
> 1) Ignore bitfield values for port based offload configuration.
It is only this meaning.
> 2) For rxq, use rx_conf.offloads field.
>
> testpmd is still using port based offload (rte_eth_rxmode) but "offloads"
> variable instead of bitfields, right?
Right.
queue specific ones are copy of port
> configs.
>
> <...>
On 12/4/2017 10:39 PM, Shahaf Shuler wrote:
> Tuesday, December 5, 2017 12:31 AM, Ferruh Yigit:
>> On 11/23/2017 4:08 AM, Shahaf Shuler wrote:
>>> Ethdev Rx/Tx offloads API has changed since:
>>>
>>> commit ce17eddefc20 ("ethdev: introduce Rx queue offloads API") commit
>>> cba7f53b717d ("ethdev: introduce Tx queue offloads API")
>>>
>>> Convert the application to use the new API.
>>
>> Hi Shahaf,
>>
>> As far as I can see patch does a few things:
>> 1- Convert rxmode bitfields usage to rxmode offloads usage.
>> 2- Apply some config options to port config and add some port config checks.
>> 3- Adding device advertised capability checks for some offloads.
>>
>> Would you mind separate 2 and 3 to their own patches, with that 1 should be
>> straightforward and 2 & 3 will be easy to review.
>
> See below comments. #2 on your list is actually needed for the conversion patch.
Please see below comment [1] for it.
> I can split the extra cap check if you fill it needs to be in a separate patch.
I find it hard to review this patch, splitting is to make easier to understand
the changes, there is no extra need.
>
>>
>>
>> And is this update tested with PMDs both support new offload method and
>> old offload method?
>
> It was tested with mlx5 and mlx4 PMD after the conversion to the new APIs.
> I put this series early so everyone can try, test and report if something is broken.
> I will try to do more testing with the old mlx PMD.
>
> BTW - we agreed that we set DD for all PMDs to move to the new API by 18.02. I still haven't see patches for that from the rest.>
>>
>> Thanks,
>> ferruh
>>
>>>
>>> Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
>>
>> <...>
>>
>>> } else if (!strcmp(res->name, "hw-vlan-extend")) {
>>> if (!strcmp(res->value, "on"))
>>> - rx_mode.hw_vlan_extend = 1;
>>> + rx_offloads |= DEV_RX_OFFLOAD_VLAN_EXTEND;
>>
>> Not related to this patch, but since you are touching these, what is the
>> difference between DEV_RX_OFFLOAD_VLAN_EXTEND and
>> DEV_RX_OFFLOAD_QINQ_STRIP ?
>
> Good question, I could not figure it out either.
> I guess those are identical. In the old API the hw_vlan_extend was the offload and the DEV_RX_OFFLOAD_QINQ_STRIP was the cap.
> Now that we merged them we have duplication.
>
> From one side, using DEV_RX_OFFLOAD_VLAN_EXTEND is more intuitive for application which previously used hw_vlan_extend to set the offload,
> For the other side QINQ_STRIP is more explicit with respect to what the feature does, and it is the flag which is currently being used for PMDs.
>
> So when we will change it, I guess I am in favor of the QINQ flag.
+1 to QINQ.
>
>>
>>> @@ -3434,7 +3439,14 @@ cmd_tx_vlan_set_parsed(void *parsed_result,
>> {
>>> struct cmd_tx_vlan_set_result *res = parsed_result;
>>>
>>> + if (!all_ports_stopped()) {
>>> + printf("Please stop all ports first\n");
>>> + return;
>>> + }
>>
>> rte_eth_rxmode bitfields to "offloads" conversion is mostly straightforward,
>> but is above kind of modifications part of this conversion or are you adding
>> missing checks?
>
> It is part of the conversion and this is related to testpmd design.
>
> Previously in the old API the tx offloads were set even after the port is started. For this specific example the vlan insert just changed a flag (TESTPMD_TX_OFFLOAD_INSERT_VLAN) on the application port struct to use PKT_TX_VLAN_PKT in the mbuf ol_flags. The application didn't update the PMD in anyway. In fact, it was possible to put txq_flag which says no vlan insertion and then to set vlan insertion through the CLI.
As you said, so I am a little concerned if missing something, because otherwise
how this was working previously?
> This was OK back then because all of the Tx offloads were set by default and we assumed users know what they are doing when setting the txq_flags.
>
> Now all the tx offloads are disabled by default. Every Tx offload being used should update the PMD (as it may need to switch tx burst function). This is why the Tx offloads configuration must be done when the port is stopped, and be followed with reconfiguration of the device and the queues.
[1]
Not agree on this part. Indeed you are fixing a behavior of the testpmd, not
just converting to new method. Technically you can provide same old config with
new flags, like enable all tx offloads, so behavior should be same.
Later can fix testpmd in another patch, this gives a better separation.
And for example I remember Konstatin mentioned some Intel NICs can accept vlan
related configuration updates without stopping the forwarding, we can discuss
these kind of things in this fix patch.
Just suggesting removing straightforward bitfield to offloads bitwise changes
out of way, and focus on what logic is changing.
>
>>
>> I would prefer making only conversion related changes in this patch, and
>> extra improvements in other patch.
>>
>>> +
>>> tx_vlan_set(res->port_id, res->vlan_id);
>>> +
>>> + cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
>>
>> Is this required for converting bitfield to offloads usage?
>
> Yes, see above.
btw, why RTE_PORT_ALL, just providing a port_id also should be OK.
>
>>
>>> }
>>>
>>> cmdline_parse_token_string_t cmd_tx_vlan_set_tx_vlan = @@ -3481,7
>>> +3493,14 @@ cmd_tx_vlan_set_qinq_parsed(void *parsed_result, {
>>> struct cmd_tx_vlan_set_qinq_result *res = parsed_result;
>>>
>>> + if (!all_ports_stopped()) {
>>> + printf("Please stop all ports first\n");
>>> + return;
>>> + }
>>
>> Same for all occurrence of these updates.
>>
>> <...>
>>
>>> @@ -3693,22 +3724,34 @@ cmd_csum_parsed(void *parsed_result,
>>>
>>> if (!strcmp(res->proto, "ip")) {
>>> mask = TESTPMD_TX_OFFLOAD_IP_CKSUM;
>>> + csum_offloads |= DEV_TX_OFFLOAD_IPV4_CKSUM;
>>> } else if (!strcmp(res->proto, "udp")) {
>>> mask = TESTPMD_TX_OFFLOAD_UDP_CKSUM;
>>> + csum_offloads |= DEV_TX_OFFLOAD_UDP_CKSUM;
>>> } else if (!strcmp(res->proto, "tcp")) {
>>> mask = TESTPMD_TX_OFFLOAD_TCP_CKSUM;
>>> + csum_offloads |= DEV_TX_OFFLOAD_TCP_CKSUM;
>>> } else if (!strcmp(res->proto, "sctp")) {
>>> mask = TESTPMD_TX_OFFLOAD_SCTP_CKSUM;
>>> + csum_offloads |= DEV_TX_OFFLOAD_SCTP_CKSUM;
>>> } else if (!strcmp(res->proto, "outer-ip")) {
>>> mask = TESTPMD_TX_OFFLOAD_OUTER_IP_CKSUM;
>>> + csum_offloads |=
>> DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM;
>>> }
>>>
>>> - if (hw)
>>> + if (hw) {
>>> ports[res->port_id].tx_ol_flags |= mask;
>>> - else
>>> + ports[res->port_id].dev_conf.txmode.offloads |=
>>> + csum_offloads;
>>
>> So you are updating port config as well as testpmd internal configuration,
>> again I guess this is not related to conversion to offloads usage.
>
> It is. See above.
> The port config will be used for the reconfiguration of the device and queues. This is a must for the Tx offloads .
>
>>
>> <...>
>>
>>> @@ -13017,6 +13093,13 @@ cmd_set_macsec_offload_on_parsed(
>>>
>>> switch (ret) {
>>> case 0:
>>> + rte_eth_dev_info_get(port_id, &dev_info);
>>> + if ((dev_info.tx_offload_capa &
>>> + DEV_TX_OFFLOAD_MACSEC_INSERT) == 0) {
>>> + printf("Warning: macsec insert enabled but not "
>>> + "supported by port %d\n", port_id);
>>> + }
>>
>> This also adding another layer of check if device advertise requested
>> capability, this is an improvement independent from conversion, can you
>> please separate into its own patch?
>
> Yes we can do it.
>
>>
>> <...>
>>
>>> @@ -606,8 +616,8 @@ port_offload_cap_display(portid_t port_id)
>>>
>>> if (dev_info.tx_offload_capa & DEV_TX_OFFLOAD_VLAN_INSERT) {
>>> printf("VLAN insert: ");
>>> - if (ports[port_id].tx_ol_flags &
>>> - TESTPMD_TX_OFFLOAD_INSERT_VLAN)
>>
>> This is removing testpmd local config check, just to double check if all places
>> that updates this local config covered to update device config variable?
>
> I hope so. If you find something I missed let me know :).
If you are not sure, and if you are not removing tx_ol_flags, what is the
benefit of replacing?
Whoever does the work removing tx_ol_flags can do replacing, this also ensures
all instances updated, no?
>
>>
>> And do we still need testpmd tx_ol_flags after these changes?
>
> Currently those flags are being used. One can prepare another patch to remove those and use the port config flags instead.
> This is not related to the conversion and could be a nice cleanup.
>
>>
>> <...>
>>
>>> @@ -1658,7 +1679,8 @@ rxtx_config_display(void)
>>> printf(" %s packet forwarding%s - CRC stripping %s - "
>>> "packets/burst=%d\n", cur_fwd_eng->fwd_mode_name,
>>> retry_enabled == 0 ? "" : " with retry",
>>> - rx_mode.hw_strip_crc ? "enabled" : "disabled",
>>> + (ports[0].dev_conf.rxmode.offloads &
>> DEV_RX_OFFLOAD_CRC_STRIP) ?
>>> + "enabled" : "disabled",
>>
>> There is a global config option in testpmd, for all ports. Previous log was print
>> based on that config option, but now you are printing the value of first port.
>
> Not exactly (there are multiple wrong issues with this function). For example the next lines are:
>
> if (cur_fwd_eng == &tx_only_engine || cur_fwd_eng == &flow_gen_engine)
> printf(" packet len=%u - nb packet segments=%d\n",
> (unsigned)tx_pkt_length, (int) tx_pkt_nb_segs);
>
> struct rte_eth_rxconf *rx_conf = &ports[0].rx_conf;
> struct rte_eth_txconf *tx_conf = &ports[0].tx_conf;
>
> the last were added by commit f2c5125a686a ("app/testpmd: use default Rx/Tx port configuration").
>
> As you can see port[0] is the one being used for the rest of the configuration print.
>
>>
>> I believe it is wrong to display only first port values, either log can be updated
>> to say testpmd default configs, or remove completely, or print for all ports,
>> what do you think?
>
> IMO it is the best to print for all ports.
+1, can this fix be part of this patchset although it is not directly related to
the offload conversion?
>
>>
>> <...>
>>
>>> @@ -897,34 +900,31 @@ launch_args_parse(int argc, char** argv)
>>> }
>>> #endif
>>> if (!strcmp(lgopts[opt_idx].name, "disable-crc-
>> strip"))
>>> - rx_mode.hw_strip_crc = 0;
>>> + rx_offloads &=
>> ~DEV_RX_OFFLOAD_CRC_STRIP;
>>> if (!strcmp(lgopts[opt_idx].name, "enable-lro"))
>>> - rx_mode.enable_lro = 1;
>>> - if (!strcmp(lgopts[opt_idx].name, "enable-scatter"))
>>> - rx_mode.enable_scatter = 1;
>>> + rx_offloads |= DEV_RX_OFFLOAD_TCP_LRO;
>>> + if (!strcmp(lgopts[opt_idx].name, "enable-scatter"))
>> {
>>> + rx_offloads |= DEV_RX_OFFLOAD_SCATTER;
>>> + }
>>
>> Can drop "{}"
>
> Yes.
>
>>
>> <...>
>>
>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>>> c3ab44849..e3a7c26b8 100644
>>> --- a/app/test-pmd/testpmd.c
>>> +++ b/app/test-pmd/testpmd.c
>>> @@ -338,15 +338,10 @@ lcoreid_t latencystats_lcore_id = -1;
>>> */
>>> struct rte_eth_rxmode rx_mode = {
>>> .max_rx_pkt_len = ETHER_MAX_LEN, /**< Default maximum frame
>> length. */
>>> - .split_hdr_size = 0,
>>> - .header_split = 0, /**< Header Split disabled. */
>>> - .hw_ip_checksum = 0, /**< IP checksum offload disabled. */
>>> - .hw_vlan_filter = 1, /**< VLAN filtering enabled. */
>>> - .hw_vlan_strip = 1, /**< VLAN strip enabled. */
>>> - .hw_vlan_extend = 0, /**< Extended VLAN disabled. */
>>> - .jumbo_frame = 0, /**< Jumbo Frame Support disabled. */
>>> - .hw_strip_crc = 1, /**< CRC stripping by hardware enabled. */
>>> - .hw_timestamp = 0, /**< HW timestamp enabled. */
>>> + .offloads = (DEV_RX_OFFLOAD_VLAN_FILTER |
>>> + DEV_RX_OFFLOAD_VLAN_STRIP |
>>> + DEV_RX_OFFLOAD_CRC_STRIP),
>>> + .ignore_offload_bitfield = 1, /**< Use rte_eth_rxq_conf offloads API
>>> +*/
>>
>> Is comment correct?
>
> No I should remove it.
>
>> Flag has two meaning I guess,
>> 1) Ignore bitfield values for port based offload configuration.
>
> It is only this meaning.
>
>> 2) For rxq, use rx_conf.offloads field.
>>
>> testpmd is still using port based offload (rte_eth_rxmode) but "offloads"
>> variable instead of bitfields, right?
>
> Right.
>
> queue specific ones are copy of port
>> configs.
>>
>> <...>
> @@ -1495,6 +1490,10 @@ start_port(portid_t pid)
> }
> if (port->need_reconfig_queues > 0) {
> port->need_reconfig_queues = 0;
> + /* Use rte_eth_txq_conf offloads API */
> + port->tx_conf.txq_flags = ETH_TXQ_FLAGS_IGNORE;
Also I just catch this part, during app start txq_flags set via PMD default
values (rxtx_port_config), if you overwrite this flag without converting to
offloads, for PMDs supporting old method you are loosing configuration.
> + /* Apply Tx offloads configuration */
> + port->tx_conf.offloads = port->dev_conf.txmode.offloads;
> /* setup tx queues */
> for (qi = 0; qi < nb_txq; qi++) {
> if ((numa_support) &&
> @@ -1521,6 +1520,8 @@ start_port(portid_t pid)
> port->need_reconfig_queues = 1;
> return -1;
> }
> + /* Apply Rx offloads configuration */
> + port->rx_conf.offloads = port->dev_conf.rxmode.offloads;
> /* setup rx queues */
> for (qi = 0; qi < nb_rxq; qi++) {
> if ((numa_support) &&
>
Thursday, December 7, 2017 12:58 AM, Ferruh Yigit:
> On 12/4/2017 10:39 PM, Shahaf Shuler wrote:
> > Tuesday, December 5, 2017 12:31 AM, Ferruh Yigit:
> >> On 11/23/2017 4:08 AM, Shahaf Shuler wrote:
> >>> Ethdev Rx/Tx offloads API has changed since:
> >>>
> >>> commit ce17eddefc20 ("ethdev: introduce Rx queue offloads API")
> >>> commit cba7f53b717d ("ethdev: introduce Tx queue offloads API")
> >>>
> >>> Convert the application to use the new API.
> >>
> >> Hi Shahaf,
> >>
> >> As far as I can see patch does a few things:
> >> 1- Convert rxmode bitfields usage to rxmode offloads usage.
> >> 2- Apply some config options to port config and add some port config
> checks.
> >> 3- Adding device advertised capability checks for some offloads.
> >>
> >> Would you mind separate 2 and 3 to their own patches, with that 1
> >> should be straightforward and 2 & 3 will be easy to review.
> >
> > See below comments. #2 on your list is actually needed for the conversion
> patch.
>
> Please see below comment [1] for it.
>
> > I can split the extra cap check if you fill it needs to be in a separate patch.
>
> I find it hard to review this patch, splitting is to make easier to understand the
> changes, there is no extra need.
OK will have them splitted on v2.
>
> >
> >>
> >>
> >> And is this update tested with PMDs both support new offload method
> >> and old offload method?
> >
> > It was tested with mlx5 and mlx4 PMD after the conversion to the new
> APIs.
> > I put this series early so everyone can try, test and report if something is
> broken.
> > I will try to do more testing with the old mlx PMD.
> >
> > BTW - we agreed that we set DD for all PMDs to move to the new API by
> > 18.02. I still haven't see patches for that from the rest.>
> >>
> >> Thanks,
> >> ferruh
> >>
> >>>
> >>> Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> >>
> >> <...>
> >>
> >>> } else if (!strcmp(res->name, "hw-vlan-extend")) {
> >>> if (!strcmp(res->value, "on"))
> >>> - rx_mode.hw_vlan_extend = 1;
> >>> + rx_offloads |= DEV_RX_OFFLOAD_VLAN_EXTEND;
> >>
> >> Not related to this patch, but since you are touching these, what is
> >> the difference between DEV_RX_OFFLOAD_VLAN_EXTEND and
> >> DEV_RX_OFFLOAD_QINQ_STRIP ?
> >
> > Good question, I could not figure it out either.
> > I guess those are identical. In the old API the hw_vlan_extend was the
> offload and the DEV_RX_OFFLOAD_QINQ_STRIP was the cap.
> > Now that we merged them we have duplication.
> >
> > From one side, using DEV_RX_OFFLOAD_VLAN_EXTEND is more intuitive
> for
> > application which previously used hw_vlan_extend to set the offload, For
> the other side QINQ_STRIP is more explicit with respect to what the feature
> does, and it is the flag which is currently being used for PMDs.
> >
> > So when we will change it, I guess I am in favor of the QINQ flag.
>
> +1 to QINQ.
>
> >
> >>
> >>> @@ -3434,7 +3439,14 @@ cmd_tx_vlan_set_parsed(void
> *parsed_result,
> >> {
> >>> struct cmd_tx_vlan_set_result *res = parsed_result;
> >>>
> >>> + if (!all_ports_stopped()) {
> >>> + printf("Please stop all ports first\n");
> >>> + return;
> >>> + }
> >>
> >> rte_eth_rxmode bitfields to "offloads" conversion is mostly
> >> straightforward, but is above kind of modifications part of this
> >> conversion or are you adding missing checks?
> >
> > It is part of the conversion and this is related to testpmd design.
> >
> > Previously in the old API the tx offloads were set even after the port is
> started. For this specific example the vlan insert just changed a flag
> (TESTPMD_TX_OFFLOAD_INSERT_VLAN) on the application port struct to use
> PKT_TX_VLAN_PKT in the mbuf ol_flags. The application didn't update the
> PMD in anyway. In fact, it was possible to put txq_flag which says no vlan
> insertion and then to set vlan insertion through the CLI.
>
> As you said, so I am a little concerned if missing something, because
> otherwise how this was working previously?
Again - because all of the tx offload where enabled by default. This means that the PMDs which supports those offload were using the tx/rx_burst functions which includes the functionally (unless the application disable those offloads using txq_flags). There was no need to futher update the PMD on the Tx offloads being used.
As for the conflicts between CLI commands and txq flags - small bug in PMD which wasn't discovered probably because no one did some strange configuration.
>
> > This was OK back then because all of the Tx offloads were set by default
> and we assumed users know what they are doing when setting the
> txq_flags.
> >
> > Now all the tx offloads are disabled by default. Every Tx offload being used
> should update the PMD (as it may need to switch tx burst function). This is
> why the Tx offloads configuration must be done when the port is stopped,
> and be followed with reconfiguration of the device and the queues.
>
> [1]
> Not agree on this part. Indeed you are fixing a behavior of the testpmd, not
> just converting to new method. Technically you can provide same old config
> with new flags, like enable all tx offloads, so behavior should be same.
>
> Later can fix testpmd in another patch, this gives a better separation.
>
> And for example I remember Konstatin mentioned some Intel NICs can
> accept vlan related configuration updates without stopping the forwarding,
> we can discuss these kind of things in this fix patch.
The on the flight VLAN configuration was converted as well and supported. I will make it more explicit on v2 with a separate patch.
>
> Just suggesting removing straightforward bitfield to offloads bitwise changes
> out of way, and focus on what logic is changing.
>
> >
> >>
> >> I would prefer making only conversion related changes in this patch,
> >> and extra improvements in other patch.
> >>
> >>> +
> >>> tx_vlan_set(res->port_id, res->vlan_id);
> >>> +
> >>> + cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
> >>
> >> Is this required for converting bitfield to offloads usage?
> >
> > Yes, see above.
>
> btw, why RTE_PORT_ALL, just providing a port_id also should be OK.
Yeah, I will switch to port_id.
>
> >
> >>
> >>> }
> >>>
> >>> cmdline_parse_token_string_t cmd_tx_vlan_set_tx_vlan = @@ -3481,7
> >>> +3493,14 @@ cmd_tx_vlan_set_qinq_parsed(void *parsed_result, {
> >>> struct cmd_tx_vlan_set_qinq_result *res = parsed_result;
> >>>
> >>> + if (!all_ports_stopped()) {
> >>> + printf("Please stop all ports first\n");
> >>> + return;
> >>> + }
> >>
> >> Same for all occurrence of these updates.
> >>
> >> <...>
> >>
> >>> @@ -3693,22 +3724,34 @@ cmd_csum_parsed(void *parsed_result,
> >>>
> >>> if (!strcmp(res->proto, "ip")) {
> >>> mask = TESTPMD_TX_OFFLOAD_IP_CKSUM;
> >>> + csum_offloads |= DEV_TX_OFFLOAD_IPV4_CKSUM;
> >>> } else if (!strcmp(res->proto, "udp")) {
> >>> mask = TESTPMD_TX_OFFLOAD_UDP_CKSUM;
> >>> + csum_offloads |= DEV_TX_OFFLOAD_UDP_CKSUM;
> >>> } else if (!strcmp(res->proto, "tcp")) {
> >>> mask = TESTPMD_TX_OFFLOAD_TCP_CKSUM;
> >>> + csum_offloads |= DEV_TX_OFFLOAD_TCP_CKSUM;
> >>> } else if (!strcmp(res->proto, "sctp")) {
> >>> mask = TESTPMD_TX_OFFLOAD_SCTP_CKSUM;
> >>> + csum_offloads |= DEV_TX_OFFLOAD_SCTP_CKSUM;
> >>> } else if (!strcmp(res->proto, "outer-ip")) {
> >>> mask = TESTPMD_TX_OFFLOAD_OUTER_IP_CKSUM;
> >>> + csum_offloads |=
> >> DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM;
> >>> }
> >>>
> >>> - if (hw)
> >>> + if (hw) {
> >>> ports[res->port_id].tx_ol_flags |= mask;
> >>> - else
> >>> + ports[res->port_id].dev_conf.txmode.offloads |=
> >>> + csum_offloads;
> >>
> >> So you are updating port config as well as testpmd internal
> >> configuration, again I guess this is not related to conversion to offloads
> usage.
> >
> > It is. See above.
> > The port config will be used for the reconfiguration of the device and
> queues. This is a must for the Tx offloads .
> >
> >>
> >> <...>
> >>
> >>> @@ -13017,6 +13093,13 @@ cmd_set_macsec_offload_on_parsed(
> >>>
> >>> switch (ret) {
> >>> case 0:
> >>> + rte_eth_dev_info_get(port_id, &dev_info);
> >>> + if ((dev_info.tx_offload_capa &
> >>> + DEV_TX_OFFLOAD_MACSEC_INSERT) == 0) {
> >>> + printf("Warning: macsec insert enabled but not "
> >>> + "supported by port %d\n", port_id);
> >>> + }
> >>
> >> This also adding another layer of check if device advertise requested
> >> capability, this is an improvement independent from conversion, can
> >> you please separate into its own patch?
> >
> > Yes we can do it.
> >
> >>
> >> <...>
> >>
> >>> @@ -606,8 +616,8 @@ port_offload_cap_display(portid_t port_id)
> >>>
> >>> if (dev_info.tx_offload_capa & DEV_TX_OFFLOAD_VLAN_INSERT) {
> >>> printf("VLAN insert: ");
> >>> - if (ports[port_id].tx_ol_flags &
> >>> - TESTPMD_TX_OFFLOAD_INSERT_VLAN)
> >>
> >> This is removing testpmd local config check, just to double check if
> >> all places that updates this local config covered to update device config
> variable?
> >
> > I hope so. If you find something I missed let me know :).
>
> If you are not sure, and if you are not removing tx_ol_flags, what is the
> benefit of replacing?
>
> Whoever does the work removing tx_ol_flags can do replacing, this also
> ensures all instances updated, no?
Will provide another patch to remove the ol_flags.
>
> >
> >>
> >> And do we still need testpmd tx_ol_flags after these changes?
> >
> > Currently those flags are being used. One can prepare another patch to
> remove those and use the port config flags instead.
> > This is not related to the conversion and could be a nice cleanup.
> >
> >>
> >> <...>
> >>
> >>> @@ -1658,7 +1679,8 @@ rxtx_config_display(void)
> >>> printf(" %s packet forwarding%s - CRC stripping %s - "
> >>> "packets/burst=%d\n", cur_fwd_eng->fwd_mode_name,
> >>> retry_enabled == 0 ? "" : " with retry",
> >>> - rx_mode.hw_strip_crc ? "enabled" : "disabled",
> >>> + (ports[0].dev_conf.rxmode.offloads &
> >> DEV_RX_OFFLOAD_CRC_STRIP) ?
> >>> + "enabled" : "disabled",
> >>
> >> There is a global config option in testpmd, for all ports. Previous
> >> log was print based on that config option, but now you are printing the
> value of first port.
> >
> > Not exactly (there are multiple wrong issues with this function). For
> example the next lines are:
> >
> > if (cur_fwd_eng == &tx_only_engine || cur_fwd_eng ==
> &flow_gen_engine)
> > printf(" packet len=%u - nb packet segments=%d\n",
> > (unsigned)tx_pkt_length, (int)
> > tx_pkt_nb_segs);
> >
> > struct rte_eth_rxconf *rx_conf = &ports[0].rx_conf;
> > struct rte_eth_txconf *tx_conf = &ports[0].tx_conf;
> >
> > the last were added by commit f2c5125a686a ("app/testpmd: use default
> Rx/Tx port configuration").
> >
> > As you can see port[0] is the one being used for the rest of the
> configuration print.
> >
> >>
> >> I believe it is wrong to display only first port values, either log
> >> can be updated to say testpmd default configs, or remove completely,
> >> or print for all ports, what do you think?
> >
> > IMO it is the best to print for all ports.
>
> +1, can this fix be part of this patchset although it is not directly
> +related to
> the offload conversion?
It can.
>
> >
> >>
> >> <...>
> >>
> >>> @@ -897,34 +900,31 @@ launch_args_parse(int argc, char** argv)
> >>> }
> >>> #endif
> >>> if (!strcmp(lgopts[opt_idx].name, "disable-crc-
> >> strip"))
> >>> - rx_mode.hw_strip_crc = 0;
> >>> + rx_offloads &=
> >> ~DEV_RX_OFFLOAD_CRC_STRIP;
> >>> if (!strcmp(lgopts[opt_idx].name, "enable-lro"))
> >>> - rx_mode.enable_lro = 1;
> >>> - if (!strcmp(lgopts[opt_idx].name, "enable-scatter"))
> >>> - rx_mode.enable_scatter = 1;
> >>> + rx_offloads |= DEV_RX_OFFLOAD_TCP_LRO;
> >>> + if (!strcmp(lgopts[opt_idx].name, "enable-scatter"))
> >> {
> >>> + rx_offloads |= DEV_RX_OFFLOAD_SCATTER;
> >>> + }
> >>
> >> Can drop "{}"
> >
> > Yes.
> >
> >>
> >> <...>
> >>
> >>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> >>> c3ab44849..e3a7c26b8 100644
> >>> --- a/app/test-pmd/testpmd.c
> >>> +++ b/app/test-pmd/testpmd.c
> >>> @@ -338,15 +338,10 @@ lcoreid_t latencystats_lcore_id = -1;
> >>> */
> >>> struct rte_eth_rxmode rx_mode = {
> >>> .max_rx_pkt_len = ETHER_MAX_LEN, /**< Default maximum frame
> >> length. */
> >>> - .split_hdr_size = 0,
> >>> - .header_split = 0, /**< Header Split disabled. */
> >>> - .hw_ip_checksum = 0, /**< IP checksum offload disabled. */
> >>> - .hw_vlan_filter = 1, /**< VLAN filtering enabled. */
> >>> - .hw_vlan_strip = 1, /**< VLAN strip enabled. */
> >>> - .hw_vlan_extend = 0, /**< Extended VLAN disabled. */
> >>> - .jumbo_frame = 0, /**< Jumbo Frame Support disabled. */
> >>> - .hw_strip_crc = 1, /**< CRC stripping by hardware enabled. */
> >>> - .hw_timestamp = 0, /**< HW timestamp enabled. */
> >>> + .offloads = (DEV_RX_OFFLOAD_VLAN_FILTER |
> >>> + DEV_RX_OFFLOAD_VLAN_STRIP |
> >>> + DEV_RX_OFFLOAD_CRC_STRIP),
> >>> + .ignore_offload_bitfield = 1, /**< Use rte_eth_rxq_conf offloads
> >>> +API */
> >>
> >> Is comment correct?
> >
> > No I should remove it.
> >
> >> Flag has two meaning I guess,
> >> 1) Ignore bitfield values for port based offload configuration.
> >
> > It is only this meaning.
> >
> >> 2) For rxq, use rx_conf.offloads field.
> >>
> >> testpmd is still using port based offload (rte_eth_rxmode) but "offloads"
> >> variable instead of bitfields, right?
> >
> > Right.
> >
> > queue specific ones are copy of port
> >> configs.
> >>
> >> <...>
>
> > @@ -1495,6 +1490,10 @@ start_port(portid_t pid)
> > }
> > if (port->need_reconfig_queues > 0) {
> > port->need_reconfig_queues = 0;
> > + /* Use rte_eth_txq_conf offloads API */
> > + port->tx_conf.txq_flags = ETH_TXQ_FLAGS_IGNORE;
>
> Also I just catch this part, during app start txq_flags set via PMD default
> values (rxtx_port_config), if you overwrite this flag without converting to
> offloads, for PMDs supporting old method you are loosing configuration.
Am disagree.
The txq_flags are to disable offloads. From application which uses the new API perspective they are all disabled. With the new offloads API, the application is the one to choose the offloads being used, not the PMD therefore there is no point with converting the PMD txq_flags.
If the underlying PMD is not supporting the new API, the offloads flags selected by the application will be converted into txq_flags. For example 0 will be converted to all of the TXQ_FLAGS.
It is true that before the changes we could still have some offloads enabled (like TSO) when we use the PMD default txq_flags. However I see it as an improvement and not downside. The new application is better because it don't have offloads enabled for nothing.
>
> > + /* Apply Tx offloads configuration */
> > + port->tx_conf.offloads = port-
> >dev_conf.txmode.offloads;
> > /* setup tx queues */
> > for (qi = 0; qi < nb_txq; qi++) {
> > if ((numa_support) &&
> > @@ -1521,6 +1520,8 @@ start_port(portid_t pid)
> > port->need_reconfig_queues = 1;
> > return -1;
> > }
> > + /* Apply Rx offloads configuration */
> > + port->rx_conf.offloads = port-
> >dev_conf.rxmode.offloads;
> > /* setup rx queues */
> > for (qi = 0; qi < nb_rxq; qi++) {
> > if ((numa_support) &&
> >
@@ -1577,6 +1577,7 @@ cmd_config_max_pkt_len_parsed(void *parsed_result,
__attribute__((unused)) void *data)
{
struct cmd_config_max_pkt_len_result *res = parsed_result;
+ uint64_t rx_offloads = rx_mode.offloads;
if (!all_ports_stopped()) {
printf("Please stop all ports first\n");
@@ -1594,14 +1595,16 @@ cmd_config_max_pkt_len_parsed(void *parsed_result,
rx_mode.max_rx_pkt_len = res->value;
if (res->value > ETHER_MAX_LEN)
- rx_mode.jumbo_frame = 1;
+ rx_offloads |= DEV_RX_OFFLOAD_JUMBO_FRAME;
else
- rx_mode.jumbo_frame = 0;
+ rx_offloads &= ~DEV_RX_OFFLOAD_JUMBO_FRAME;
} else {
printf("Unknown parameter\n");
return;
}
+ rx_mode.offloads = rx_offloads;
+
init_port_config();
cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
@@ -1703,6 +1706,7 @@ cmd_config_rx_mode_flag_parsed(void *parsed_result,
__attribute__((unused)) void *data)
{
struct cmd_config_rx_mode_flag *res = parsed_result;
+ uint64_t rx_offloads = rx_mode.offloads;
if (!all_ports_stopped()) {
printf("Please stop all ports first\n");
@@ -1711,48 +1715,48 @@ cmd_config_rx_mode_flag_parsed(void *parsed_result,
if (!strcmp(res->name, "crc-strip")) {
if (!strcmp(res->value, "on"))
- rx_mode.hw_strip_crc = 1;
+ rx_offloads |= DEV_RX_OFFLOAD_CRC_STRIP;
else if (!strcmp(res->value, "off"))
- rx_mode.hw_strip_crc = 0;
+ rx_offloads &= ~DEV_RX_OFFLOAD_CRC_STRIP;
else {
printf("Unknown parameter\n");
return;
}
} else if (!strcmp(res->name, "scatter")) {
- if (!strcmp(res->value, "on"))
- rx_mode.enable_scatter = 1;
- else if (!strcmp(res->value, "off"))
- rx_mode.enable_scatter = 0;
- else {
+ if (!strcmp(res->value, "on")) {
+ rx_offloads |= DEV_RX_OFFLOAD_SCATTER;
+ } else if (!strcmp(res->value, "off")) {
+ rx_offloads &= ~DEV_RX_OFFLOAD_SCATTER;
+ } else {
printf("Unknown parameter\n");
return;
}
} else if (!strcmp(res->name, "rx-cksum")) {
if (!strcmp(res->value, "on"))
- rx_mode.hw_ip_checksum = 1;
+ rx_offloads |= DEV_RX_OFFLOAD_CHECKSUM;
else if (!strcmp(res->value, "off"))
- rx_mode.hw_ip_checksum = 0;
+ rx_offloads &= ~DEV_RX_OFFLOAD_CHECKSUM;
else {
printf("Unknown parameter\n");
return;
}
} else if (!strcmp(res->name, "rx-timestamp")) {
if (!strcmp(res->value, "on"))
- rx_mode.hw_timestamp = 1;
+ rx_offloads |= DEV_RX_OFFLOAD_TIMESTAMP;
else if (!strcmp(res->value, "off"))
- rx_mode.hw_timestamp = 0;
+ rx_offloads &= ~DEV_RX_OFFLOAD_TIMESTAMP;
else {
printf("Unknown parameter\n");
return;
}
} else if (!strcmp(res->name, "hw-vlan")) {
if (!strcmp(res->value, "on")) {
- rx_mode.hw_vlan_filter = 1;
- rx_mode.hw_vlan_strip = 1;
+ rx_offloads |= (DEV_RX_OFFLOAD_VLAN_FILTER |
+ DEV_RX_OFFLOAD_VLAN_STRIP);
}
else if (!strcmp(res->value, "off")) {
- rx_mode.hw_vlan_filter = 0;
- rx_mode.hw_vlan_strip = 0;
+ rx_offloads &= ~(DEV_RX_OFFLOAD_VLAN_FILTER |
+ DEV_RX_OFFLOAD_VLAN_STRIP);
}
else {
printf("Unknown parameter\n");
@@ -1760,27 +1764,27 @@ cmd_config_rx_mode_flag_parsed(void *parsed_result,
}
} else if (!strcmp(res->name, "hw-vlan-filter")) {
if (!strcmp(res->value, "on"))
- rx_mode.hw_vlan_filter = 1;
+ rx_offloads |= DEV_RX_OFFLOAD_VLAN_FILTER;
else if (!strcmp(res->value, "off"))
- rx_mode.hw_vlan_filter = 0;
+ rx_offloads &= ~DEV_RX_OFFLOAD_VLAN_FILTER;
else {
printf("Unknown parameter\n");
return;
}
} else if (!strcmp(res->name, "hw-vlan-strip")) {
if (!strcmp(res->value, "on"))
- rx_mode.hw_vlan_strip = 1;
+ rx_offloads |= DEV_RX_OFFLOAD_VLAN_STRIP;
else if (!strcmp(res->value, "off"))
- rx_mode.hw_vlan_strip = 0;
+ rx_offloads &= ~DEV_RX_OFFLOAD_VLAN_STRIP;
else {
printf("Unknown parameter\n");
return;
}
} else if (!strcmp(res->name, "hw-vlan-extend")) {
if (!strcmp(res->value, "on"))
- rx_mode.hw_vlan_extend = 1;
+ rx_offloads |= DEV_RX_OFFLOAD_VLAN_EXTEND;
else if (!strcmp(res->value, "off"))
- rx_mode.hw_vlan_extend = 0;
+ rx_offloads &= ~DEV_RX_OFFLOAD_VLAN_EXTEND;
else {
printf("Unknown parameter\n");
return;
@@ -1798,6 +1802,7 @@ cmd_config_rx_mode_flag_parsed(void *parsed_result,
printf("Unknown parameter\n");
return;
}
+ rx_mode.offloads = rx_offloads;
init_port_config();
@@ -3434,7 +3439,14 @@ cmd_tx_vlan_set_parsed(void *parsed_result,
{
struct cmd_tx_vlan_set_result *res = parsed_result;
+ if (!all_ports_stopped()) {
+ printf("Please stop all ports first\n");
+ return;
+ }
+
tx_vlan_set(res->port_id, res->vlan_id);
+
+ cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
}
cmdline_parse_token_string_t cmd_tx_vlan_set_tx_vlan =
@@ -3481,7 +3493,14 @@ cmd_tx_vlan_set_qinq_parsed(void *parsed_result,
{
struct cmd_tx_vlan_set_qinq_result *res = parsed_result;
+ if (!all_ports_stopped()) {
+ printf("Please stop all ports first\n");
+ return;
+ }
+
tx_qinq_set(res->port_id, res->vlan_id, res->vlan_id_outer);
+
+ cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
}
cmdline_parse_token_string_t cmd_tx_vlan_set_qinq_tx_vlan =
@@ -3587,7 +3606,14 @@ cmd_tx_vlan_reset_parsed(void *parsed_result,
{
struct cmd_tx_vlan_reset_result *res = parsed_result;
+ if (!all_ports_stopped()) {
+ printf("Please stop all ports first\n");
+ return;
+ }
+
tx_vlan_reset(res->port_id);
+
+ cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
}
cmdline_parse_token_string_t cmd_tx_vlan_reset_tx_vlan =
@@ -3680,11 +3706,16 @@ cmd_csum_parsed(void *parsed_result,
struct cmd_csum_result *res = parsed_result;
int hw = 0;
uint16_t mask = 0;
+ uint64_t csum_offloads = 0;
if (port_id_is_invalid(res->port_id, ENABLED_WARN)) {
printf("invalid port %d\n", res->port_id);
return;
}
+ if (!all_ports_stopped()) {
+ printf("Please stop all ports first\n");
+ return;
+ }
if (!strcmp(res->mode, "set")) {
@@ -3693,22 +3724,34 @@ cmd_csum_parsed(void *parsed_result,
if (!strcmp(res->proto, "ip")) {
mask = TESTPMD_TX_OFFLOAD_IP_CKSUM;
+ csum_offloads |= DEV_TX_OFFLOAD_IPV4_CKSUM;
} else if (!strcmp(res->proto, "udp")) {
mask = TESTPMD_TX_OFFLOAD_UDP_CKSUM;
+ csum_offloads |= DEV_TX_OFFLOAD_UDP_CKSUM;
} else if (!strcmp(res->proto, "tcp")) {
mask = TESTPMD_TX_OFFLOAD_TCP_CKSUM;
+ csum_offloads |= DEV_TX_OFFLOAD_TCP_CKSUM;
} else if (!strcmp(res->proto, "sctp")) {
mask = TESTPMD_TX_OFFLOAD_SCTP_CKSUM;
+ csum_offloads |= DEV_TX_OFFLOAD_SCTP_CKSUM;
} else if (!strcmp(res->proto, "outer-ip")) {
mask = TESTPMD_TX_OFFLOAD_OUTER_IP_CKSUM;
+ csum_offloads |= DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM;
}
- if (hw)
+ if (hw) {
ports[res->port_id].tx_ol_flags |= mask;
- else
+ ports[res->port_id].dev_conf.txmode.offloads |=
+ csum_offloads;
+ } else {
ports[res->port_id].tx_ol_flags &= (~mask);
+ ports[res->port_id].dev_conf.txmode.offloads &=
+ (~csum_offloads);
+ }
}
csum_show(res->port_id);
+
+ cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
}
cmdline_parse_token_string_t cmd_csum_csum =
@@ -3832,15 +3875,24 @@ cmd_tso_set_parsed(void *parsed_result,
if (port_id_is_invalid(res->port_id, ENABLED_WARN))
return;
+ if (!all_ports_stopped()) {
+ printf("Please stop all ports first\n");
+ return;
+ }
if (!strcmp(res->mode, "set"))
ports[res->port_id].tso_segsz = res->tso_segsz;
- if (ports[res->port_id].tso_segsz == 0)
+ if (ports[res->port_id].tso_segsz == 0) {
+ ports[res->port_id].dev_conf.txmode.offloads &=
+ ~DEV_TX_OFFLOAD_TCP_TSO;
printf("TSO for non-tunneled packets is disabled\n");
- else
+ } else {
+ ports[res->port_id].dev_conf.txmode.offloads |=
+ DEV_TX_OFFLOAD_TCP_TSO;
printf("TSO segment size for non-tunneled packets is %d\n",
ports[res->port_id].tso_segsz);
+ }
/* display warnings if configuration is not supported by the NIC */
rte_eth_dev_info_get(res->port_id, &dev_info);
@@ -3849,6 +3901,8 @@ cmd_tso_set_parsed(void *parsed_result,
printf("Warning: TSO enabled but not "
"supported by port %d\n", res->port_id);
}
+
+ cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
}
cmdline_parse_token_string_t cmd_tso_set_tso =
@@ -3934,13 +3988,27 @@ cmd_tunnel_tso_set_parsed(void *parsed_result,
if (port_id_is_invalid(res->port_id, ENABLED_WARN))
return;
+ if (!all_ports_stopped()) {
+ printf("Please stop all ports first\n");
+ return;
+ }
if (!strcmp(res->mode, "set"))
ports[res->port_id].tunnel_tso_segsz = res->tso_segsz;
- if (ports[res->port_id].tunnel_tso_segsz == 0)
+ if (ports[res->port_id].tunnel_tso_segsz == 0) {
+ ports[res->port_id].dev_conf.txmode.offloads &=
+ ~(DEV_TX_OFFLOAD_VXLAN_TNL_TSO |
+ DEV_TX_OFFLOAD_GRE_TNL_TSO |
+ DEV_TX_OFFLOAD_IPIP_TNL_TSO |
+ DEV_TX_OFFLOAD_GENEVE_TNL_TSO);
printf("TSO for tunneled packets is disabled\n");
- else {
+ } else {
+ ports[res->port_id].dev_conf.txmode.offloads |=
+ (DEV_TX_OFFLOAD_VXLAN_TNL_TSO |
+ DEV_TX_OFFLOAD_GRE_TNL_TSO |
+ DEV_TX_OFFLOAD_IPIP_TNL_TSO |
+ DEV_TX_OFFLOAD_GENEVE_TNL_TSO);
printf("TSO segment size for tunneled packets is %d\n",
ports[res->port_id].tunnel_tso_segsz);
@@ -3966,6 +4034,8 @@ cmd_tunnel_tso_set_parsed(void *parsed_result,
printf("Warning: csum set outer-ip must be set to hw "
"if outer L3 is IPv4; not necessary for IPv6\n");
}
+
+ cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
}
cmdline_parse_token_string_t cmd_tunnel_tso_set_tso =
@@ -13004,11 +13074,17 @@ cmd_set_macsec_offload_on_parsed(
portid_t port_id = res->port_id;
int en = (strcmp(res->en_on_off, "on") == 0) ? 1 : 0;
int rp = (strcmp(res->rp_on_off, "on") == 0) ? 1 : 0;
+ struct rte_eth_dev_info dev_info;
if (port_id_is_invalid(port_id, ENABLED_WARN))
return;
+ if (!all_ports_stopped()) {
+ printf("Please stop all ports first\n");
+ return;
+ }
ports[port_id].tx_ol_flags |= TESTPMD_TX_OFFLOAD_MACSEC;
+ ports[port_id].dev_conf.txmode.offloads |= DEV_TX_OFFLOAD_MACSEC_INSERT;
#ifdef RTE_LIBRTE_IXGBE_PMD
ret = rte_pmd_ixgbe_macsec_enable(port_id, en, rp);
#endif
@@ -13017,6 +13093,13 @@ cmd_set_macsec_offload_on_parsed(
switch (ret) {
case 0:
+ rte_eth_dev_info_get(port_id, &dev_info);
+ if ((dev_info.tx_offload_capa &
+ DEV_TX_OFFLOAD_MACSEC_INSERT) == 0) {
+ printf("Warning: macsec insert enabled but not "
+ "supported by port %d\n", port_id);
+ }
+ cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
break;
case -ENODEV:
printf("invalid port_id %d\n", port_id);
@@ -13091,14 +13174,21 @@ cmd_set_macsec_offload_off_parsed(
if (port_id_is_invalid(port_id, ENABLED_WARN))
return;
+ if (!all_ports_stopped()) {
+ printf("Please stop all ports first\n");
+ return;
+ }
ports[port_id].tx_ol_flags &= ~TESTPMD_TX_OFFLOAD_MACSEC;
+ ports[port_id].dev_conf.txmode.offloads &=
+ ~DEV_TX_OFFLOAD_MACSEC_INSERT;
#ifdef RTE_LIBRTE_IXGBE_PMD
ret = rte_pmd_ixgbe_macsec_disable(port_id);
#endif
switch (ret) {
case 0:
+ cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
break;
case -ENODEV:
printf("invalid port_id %d\n", port_id);
@@ -540,14 +540,12 @@ port_infos_display(portid_t port_id)
void
port_offload_cap_display(portid_t port_id)
{
- struct rte_eth_dev *dev;
struct rte_eth_dev_info dev_info;
static const char *info_border = "************";
if (port_id_is_invalid(port_id, ENABLED_WARN))
return;
- dev = &rte_eth_devices[port_id];
rte_eth_dev_info_get(port_id, &dev_info);
printf("\n%s Port %d supported offload features: %s\n",
@@ -555,7 +553,8 @@ port_offload_cap_display(portid_t port_id)
if (dev_info.rx_offload_capa & DEV_RX_OFFLOAD_VLAN_STRIP) {
printf("VLAN stripped: ");
- if (dev->data->dev_conf.rxmode.hw_vlan_strip)
+ if (ports[port_id].dev_conf.rxmode.offloads &
+ DEV_RX_OFFLOAD_VLAN_STRIP)
printf("on\n");
else
printf("off\n");
@@ -563,7 +562,8 @@ port_offload_cap_display(portid_t port_id)
if (dev_info.rx_offload_capa & DEV_RX_OFFLOAD_QINQ_STRIP) {
printf("Double VLANs stripped: ");
- if (dev->data->dev_conf.rxmode.hw_vlan_extend)
+ if (ports[port_id].dev_conf.rxmode.offloads &
+ DEV_RX_OFFLOAD_VLAN_EXTEND)
printf("on\n");
else
printf("off\n");
@@ -571,7 +571,8 @@ port_offload_cap_display(portid_t port_id)
if (dev_info.rx_offload_capa & DEV_RX_OFFLOAD_IPV4_CKSUM) {
printf("RX IPv4 checksum: ");
- if (dev->data->dev_conf.rxmode.hw_ip_checksum)
+ if (ports[port_id].dev_conf.rxmode.offloads &
+ DEV_RX_OFFLOAD_IPV4_CKSUM)
printf("on\n");
else
printf("off\n");
@@ -579,7 +580,8 @@ port_offload_cap_display(portid_t port_id)
if (dev_info.rx_offload_capa & DEV_RX_OFFLOAD_UDP_CKSUM) {
printf("RX UDP checksum: ");
- if (dev->data->dev_conf.rxmode.hw_ip_checksum)
+ if (ports[port_id].dev_conf.rxmode.offloads &
+ DEV_RX_OFFLOAD_UDP_CKSUM)
printf("on\n");
else
printf("off\n");
@@ -587,18 +589,26 @@ port_offload_cap_display(portid_t port_id)
if (dev_info.rx_offload_capa & DEV_RX_OFFLOAD_TCP_CKSUM) {
printf("RX TCP checksum: ");
- if (dev->data->dev_conf.rxmode.hw_ip_checksum)
+ if (ports[port_id].dev_conf.rxmode.offloads &
+ DEV_RX_OFFLOAD_TCP_CKSUM)
printf("on\n");
else
printf("off\n");
}
- if (dev_info.rx_offload_capa & DEV_RX_OFFLOAD_OUTER_IPV4_CKSUM)
- printf("RX Outer IPv4 checksum: on");
+ if (dev_info.rx_offload_capa & DEV_RX_OFFLOAD_OUTER_IPV4_CKSUM) {
+ printf("RX Outer IPv4 checksum: ");
+ if (ports[port_id].dev_conf.rxmode.offloads &
+ DEV_RX_OFFLOAD_OUTER_IPV4_CKSUM)
+ printf("on\n");
+ else
+ printf("off\n");
+ }
if (dev_info.rx_offload_capa & DEV_RX_OFFLOAD_TCP_LRO) {
printf("Large receive offload: ");
- if (dev->data->dev_conf.rxmode.enable_lro)
+ if (ports[port_id].dev_conf.rxmode.offloads &
+ DEV_RX_OFFLOAD_TCP_LRO)
printf("on\n");
else
printf("off\n");
@@ -606,8 +616,8 @@ port_offload_cap_display(portid_t port_id)
if (dev_info.tx_offload_capa & DEV_TX_OFFLOAD_VLAN_INSERT) {
printf("VLAN insert: ");
- if (ports[port_id].tx_ol_flags &
- TESTPMD_TX_OFFLOAD_INSERT_VLAN)
+ if (ports[port_id].dev_conf.txmode.offloads &
+ DEV_TX_OFFLOAD_VLAN_INSERT)
printf("on\n");
else
printf("off\n");
@@ -615,7 +625,8 @@ port_offload_cap_display(portid_t port_id)
if (dev_info.rx_offload_capa & DEV_RX_OFFLOAD_TIMESTAMP) {
printf("HW timestamp: ");
- if (dev->data->dev_conf.rxmode.hw_timestamp)
+ if (ports[port_id].dev_conf.rxmode.offloads &
+ DEV_RX_OFFLOAD_TIMESTAMP)
printf("on\n");
else
printf("off\n");
@@ -623,8 +634,8 @@ port_offload_cap_display(portid_t port_id)
if (dev_info.tx_offload_capa & DEV_TX_OFFLOAD_QINQ_INSERT) {
printf("Double VLANs insert: ");
- if (ports[port_id].tx_ol_flags &
- TESTPMD_TX_OFFLOAD_INSERT_QINQ)
+ if (ports[port_id].dev_conf.txmode.offloads &
+ DEV_TX_OFFLOAD_QINQ_INSERT)
printf("on\n");
else
printf("off\n");
@@ -632,7 +643,8 @@ port_offload_cap_display(portid_t port_id)
if (dev_info.tx_offload_capa & DEV_TX_OFFLOAD_IPV4_CKSUM) {
printf("TX IPv4 checksum: ");
- if (ports[port_id].tx_ol_flags & TESTPMD_TX_OFFLOAD_IP_CKSUM)
+ if (ports[port_id].dev_conf.txmode.offloads &
+ DEV_TX_OFFLOAD_IPV4_CKSUM)
printf("on\n");
else
printf("off\n");
@@ -640,7 +652,8 @@ port_offload_cap_display(portid_t port_id)
if (dev_info.tx_offload_capa & DEV_TX_OFFLOAD_UDP_CKSUM) {
printf("TX UDP checksum: ");
- if (ports[port_id].tx_ol_flags & TESTPMD_TX_OFFLOAD_UDP_CKSUM)
+ if (ports[port_id].dev_conf.txmode.offloads &
+ DEV_TX_OFFLOAD_UDP_CKSUM)
printf("on\n");
else
printf("off\n");
@@ -648,7 +661,8 @@ port_offload_cap_display(portid_t port_id)
if (dev_info.tx_offload_capa & DEV_TX_OFFLOAD_TCP_CKSUM) {
printf("TX TCP checksum: ");
- if (ports[port_id].tx_ol_flags & TESTPMD_TX_OFFLOAD_TCP_CKSUM)
+ if (ports[port_id].dev_conf.txmode.offloads &
+ DEV_TX_OFFLOAD_TCP_CKSUM)
printf("on\n");
else
printf("off\n");
@@ -656,7 +670,8 @@ port_offload_cap_display(portid_t port_id)
if (dev_info.tx_offload_capa & DEV_TX_OFFLOAD_SCTP_CKSUM) {
printf("TX SCTP checksum: ");
- if (ports[port_id].tx_ol_flags & TESTPMD_TX_OFFLOAD_SCTP_CKSUM)
+ if (ports[port_id].dev_conf.txmode.offloads &
+ DEV_TX_OFFLOAD_SCTP_CKSUM)
printf("on\n");
else
printf("off\n");
@@ -664,8 +679,8 @@ port_offload_cap_display(portid_t port_id)
if (dev_info.tx_offload_capa & DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM) {
printf("TX Outer IPv4 checksum: ");
- if (ports[port_id].tx_ol_flags &
- TESTPMD_TX_OFFLOAD_OUTER_IP_CKSUM)
+ if (ports[port_id].dev_conf.txmode.offloads &
+ DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM)
printf("on\n");
else
printf("off\n");
@@ -673,7 +688,8 @@ port_offload_cap_display(portid_t port_id)
if (dev_info.tx_offload_capa & DEV_TX_OFFLOAD_TCP_TSO) {
printf("TX TCP segmentation: ");
- if (ports[port_id].tso_segsz != 0)
+ if (ports[port_id].dev_conf.txmode.offloads &
+ DEV_TX_OFFLOAD_TCP_TSO)
printf("on\n");
else
printf("off\n");
@@ -681,7 +697,8 @@ port_offload_cap_display(portid_t port_id)
if (dev_info.tx_offload_capa & DEV_TX_OFFLOAD_UDP_TSO) {
printf("TX UDP segmentation: ");
- if (ports[port_id].tso_segsz != 0)
+ if (ports[port_id].dev_conf.txmode.offloads &
+ DEV_TX_OFFLOAD_UDP_TSO)
printf("on\n");
else
printf("off\n");
@@ -689,7 +706,8 @@ port_offload_cap_display(portid_t port_id)
if (dev_info.tx_offload_capa & DEV_TX_OFFLOAD_VXLAN_TNL_TSO) {
printf("TSO for VXLAN tunnel packet: ");
- if (ports[port_id].tunnel_tso_segsz)
+ if (ports[port_id].dev_conf.txmode.offloads &
+ DEV_TX_OFFLOAD_VXLAN_TNL_TSO)
printf("on\n");
else
printf("off\n");
@@ -697,7 +715,8 @@ port_offload_cap_display(portid_t port_id)
if (dev_info.tx_offload_capa & DEV_TX_OFFLOAD_GRE_TNL_TSO) {
printf("TSO for GRE tunnel packet: ");
- if (ports[port_id].tunnel_tso_segsz)
+ if (ports[port_id].dev_conf.txmode.offloads &
+ DEV_TX_OFFLOAD_GRE_TNL_TSO)
printf("on\n");
else
printf("off\n");
@@ -705,7 +724,8 @@ port_offload_cap_display(portid_t port_id)
if (dev_info.tx_offload_capa & DEV_TX_OFFLOAD_IPIP_TNL_TSO) {
printf("TSO for IPIP tunnel packet: ");
- if (ports[port_id].tunnel_tso_segsz)
+ if (ports[port_id].dev_conf.txmode.offloads &
+ DEV_TX_OFFLOAD_IPIP_TNL_TSO)
printf("on\n");
else
printf("off\n");
@@ -713,7 +733,8 @@ port_offload_cap_display(portid_t port_id)
if (dev_info.tx_offload_capa & DEV_TX_OFFLOAD_GENEVE_TNL_TSO) {
printf("TSO for GENEVE tunnel packet: ");
- if (ports[port_id].tunnel_tso_segsz)
+ if (ports[port_id].dev_conf.txmode.offloads &
+ DEV_TX_OFFLOAD_GENEVE_TNL_TSO)
printf("on\n");
else
printf("off\n");
@@ -1658,7 +1679,8 @@ rxtx_config_display(void)
printf(" %s packet forwarding%s - CRC stripping %s - "
"packets/burst=%d\n", cur_fwd_eng->fwd_mode_name,
retry_enabled == 0 ? "" : " with retry",
- rx_mode.hw_strip_crc ? "enabled" : "disabled",
+ (ports[0].dev_conf.rxmode.offloads & DEV_RX_OFFLOAD_CRC_STRIP) ?
+ "enabled" : "disabled",
nb_pkt_per_burst);
if (cur_fwd_eng == &tx_only_engine || cur_fwd_eng == &flow_gen_engine)
@@ -2758,6 +2780,8 @@ void
tx_vlan_set(portid_t port_id, uint16_t vlan_id)
{
int vlan_offload;
+ struct rte_eth_dev_info dev_info;
+
if (port_id_is_invalid(port_id, ENABLED_WARN))
return;
if (vlan_id_is_invalid(vlan_id))
@@ -2771,13 +2795,21 @@ tx_vlan_set(portid_t port_id, uint16_t vlan_id)
tx_vlan_reset(port_id);
ports[port_id].tx_ol_flags |= TESTPMD_TX_OFFLOAD_INSERT_VLAN;
+ ports[port_id].dev_conf.txmode.offloads |= DEV_TX_OFFLOAD_VLAN_INSERT;
ports[port_id].tx_vlan_id = vlan_id;
+ rte_eth_dev_info_get(port_id, &dev_info);
+ if ((dev_info.tx_offload_capa & DEV_TX_OFFLOAD_VLAN_INSERT) == 0) {
+ printf("Warning: vlan insert enabled but not "
+ "supported by port %d\n", port_id);
+ }
}
void
tx_qinq_set(portid_t port_id, uint16_t vlan_id, uint16_t vlan_id_outer)
{
int vlan_offload;
+ struct rte_eth_dev_info dev_info;
+
if (port_id_is_invalid(port_id, ENABLED_WARN))
return;
if (vlan_id_is_invalid(vlan_id))
@@ -2793,8 +2825,14 @@ tx_qinq_set(portid_t port_id, uint16_t vlan_id, uint16_t vlan_id_outer)
tx_vlan_reset(port_id);
ports[port_id].tx_ol_flags |= TESTPMD_TX_OFFLOAD_INSERT_QINQ;
+ ports[port_id].dev_conf.txmode.offloads |= DEV_TX_OFFLOAD_QINQ_INSERT;
ports[port_id].tx_vlan_id = vlan_id;
ports[port_id].tx_vlan_id_outer = vlan_id_outer;
+ rte_eth_dev_info_get(port_id, &dev_info);
+ if ((dev_info.tx_offload_capa & DEV_TX_OFFLOAD_QINQ_INSERT) == 0) {
+ printf("Warning: qinq insert enabled but not "
+ "supported by port %d\n", port_id);
+ }
}
void
@@ -2804,6 +2842,9 @@ tx_vlan_reset(portid_t port_id)
return;
ports[port_id].tx_ol_flags &= ~(TESTPMD_TX_OFFLOAD_INSERT_VLAN |
TESTPMD_TX_OFFLOAD_INSERT_QINQ);
+ ports[port_id].dev_conf.txmode.offloads &=
+ ~(DEV_TX_OFFLOAD_VLAN_INSERT |
+ DEV_TX_OFFLOAD_QINQ_INSERT);
ports[port_id].tx_vlan_id = 0;
ports[port_id].tx_vlan_id_outer = 0;
}
@@ -566,6 +566,8 @@ launch_args_parse(int argc, char** argv)
char **argvopt;
int opt_idx;
enum { TX, RX };
+ /* Default Rx offloads for all ports. */
+ uint64_t rx_offloads = rx_mode.offloads;
static struct option lgopts[] = {
{ "help", 0, 0, 0 },
@@ -804,7 +806,8 @@ launch_args_parse(int argc, char** argv)
if (n >= ETHER_MIN_LEN) {
rx_mode.max_rx_pkt_len = (uint32_t) n;
if (n > ETHER_MAX_LEN)
- rx_mode.jumbo_frame = 1;
+ rx_offloads |=
+ DEV_RX_OFFLOAD_JUMBO_FRAME;
} else
rte_exit(EXIT_FAILURE,
"Invalid max-pkt-len=%d - should be > %d\n",
@@ -897,34 +900,31 @@ launch_args_parse(int argc, char** argv)
}
#endif
if (!strcmp(lgopts[opt_idx].name, "disable-crc-strip"))
- rx_mode.hw_strip_crc = 0;
+ rx_offloads &= ~DEV_RX_OFFLOAD_CRC_STRIP;
if (!strcmp(lgopts[opt_idx].name, "enable-lro"))
- rx_mode.enable_lro = 1;
- if (!strcmp(lgopts[opt_idx].name, "enable-scatter"))
- rx_mode.enable_scatter = 1;
+ rx_offloads |= DEV_RX_OFFLOAD_TCP_LRO;
+ if (!strcmp(lgopts[opt_idx].name, "enable-scatter")) {
+ rx_offloads |= DEV_RX_OFFLOAD_SCATTER;
+ }
if (!strcmp(lgopts[opt_idx].name, "enable-rx-cksum"))
- rx_mode.hw_ip_checksum = 1;
+ rx_offloads |= DEV_RX_OFFLOAD_CHECKSUM;
if (!strcmp(lgopts[opt_idx].name,
"enable-rx-timestamp"))
- rx_mode.hw_timestamp = 1;
-
- if (!strcmp(lgopts[opt_idx].name, "disable-hw-vlan")) {
- rx_mode.hw_vlan_filter = 0;
- rx_mode.hw_vlan_strip = 0;
- rx_mode.hw_vlan_extend = 0;
- }
+ rx_offloads |= DEV_RX_OFFLOAD_TIMESTAMP;
+ if (!strcmp(lgopts[opt_idx].name, "disable-hw-vlan"))
+ rx_offloads &= ~DEV_RX_OFFLOAD_VLAN;
if (!strcmp(lgopts[opt_idx].name,
"disable-hw-vlan-filter"))
- rx_mode.hw_vlan_filter = 0;
+ rx_offloads &= ~DEV_RX_OFFLOAD_VLAN_FILTER;
if (!strcmp(lgopts[opt_idx].name,
"disable-hw-vlan-strip"))
- rx_mode.hw_vlan_strip = 0;
+ rx_offloads &= ~DEV_RX_OFFLOAD_VLAN_STRIP;
if (!strcmp(lgopts[opt_idx].name,
"disable-hw-vlan-extend"))
- rx_mode.hw_vlan_extend = 0;
+ rx_offloads &= ~DEV_RX_OFFLOAD_VLAN_EXTEND;
if (!strcmp(lgopts[opt_idx].name, "enable-drop-en"))
rx_drop_en = 1;
@@ -1140,4 +1140,7 @@ launch_args_parse(int argc, char** argv)
break;
}
}
+
+ /* Set offload configuration from command line parameters. */
+ rx_mode.offloads = rx_offloads;
}
@@ -338,15 +338,10 @@ lcoreid_t latencystats_lcore_id = -1;
*/
struct rte_eth_rxmode rx_mode = {
.max_rx_pkt_len = ETHER_MAX_LEN, /**< Default maximum frame length. */
- .split_hdr_size = 0,
- .header_split = 0, /**< Header Split disabled. */
- .hw_ip_checksum = 0, /**< IP checksum offload disabled. */
- .hw_vlan_filter = 1, /**< VLAN filtering enabled. */
- .hw_vlan_strip = 1, /**< VLAN strip enabled. */
- .hw_vlan_extend = 0, /**< Extended VLAN disabled. */
- .jumbo_frame = 0, /**< Jumbo Frame Support disabled. */
- .hw_strip_crc = 1, /**< CRC stripping by hardware enabled. */
- .hw_timestamp = 0, /**< HW timestamp enabled. */
+ .offloads = (DEV_RX_OFFLOAD_VLAN_FILTER |
+ DEV_RX_OFFLOAD_VLAN_STRIP |
+ DEV_RX_OFFLOAD_CRC_STRIP),
+ .ignore_offload_bitfield = 1, /**< Use rte_eth_rxq_conf offloads API */
};
struct rte_fdir_conf fdir_conf = {
@@ -1495,6 +1490,10 @@ start_port(portid_t pid)
}
if (port->need_reconfig_queues > 0) {
port->need_reconfig_queues = 0;
+ /* Use rte_eth_txq_conf offloads API */
+ port->tx_conf.txq_flags = ETH_TXQ_FLAGS_IGNORE;
+ /* Apply Tx offloads configuration */
+ port->tx_conf.offloads = port->dev_conf.txmode.offloads;
/* setup tx queues */
for (qi = 0; qi < nb_txq; qi++) {
if ((numa_support) &&
@@ -1521,6 +1520,8 @@ start_port(portid_t pid)
port->need_reconfig_queues = 1;
return -1;
}
+ /* Apply Rx offloads configuration */
+ port->rx_conf.offloads = port->dev_conf.rxmode.offloads;
/* setup rx queues */
for (qi = 0; qi < nb_rxq; qi++) {
if ((numa_support) &&
@@ -1534,7 +1535,6 @@ start_port(portid_t pid)
rxring_numa[pi]);
return -1;
}
-
diag = rte_eth_rx_queue_setup(pi, qi,
nb_rxd,rxring_numa[pi],
&(port->rx_conf),mp);
@@ -2252,7 +2252,7 @@ init_port_dcb_config(portid_t pid,
retval = get_eth_dcb_conf(&port_conf, dcb_mode, num_tcs, pfc_en);
if (retval < 0)
return retval;
- port_conf.rxmode.hw_vlan_filter = 1;
+ port_conf.rxmode.offloads |= DEV_RX_OFFLOAD_VLAN_FILTER;
/**
* Write the configuration into the device.
@@ -2301,7 +2301,7 @@ init_port_dcb_config(portid_t pid,
rxtx_port_config(rte_port);
/* VLAN filter */
- rte_port->dev_conf.rxmode.hw_vlan_filter = 1;
+ rte_port->dev_conf.rxmode.offloads |= DEV_RX_OFFLOAD_VLAN_FILTER;
for (i = 0; i < RTE_DIM(vlan_tags); i++)
rx_vft_set(pid, vlan_tags[i], 1);