[v5] app/testpmd: fix setting maximum packet length
Checks
Commit Message
From: Steve Yang <stevex.yang@intel.com>
"port config all max-pkt-len" command fails because it doesn't set the
'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag properly.
Commit in the fixes line moved the 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload
flag update from 'cmd_config_max_pkt_len_parsed()' to 'init_config()'.
'init_config()' function is only called during testpmd startup, but the
flag status needs to be calculated whenever 'max_rx_pkt_len' changes.
The issue can be reproduce as [1], where the 'max-pkt-len' reduced and
'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag should be cleared but it
didn't.
Adding the 'update_jumbo_frame_offload()' helper function to update
'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag and 'max_rx_pkt_len'. This
function is called both by 'init_config()' and
'cmd_config_max_pkt_len_parsed()'.
Default 'max-pkt-len' value set to zero, 'update_jumbo_frame_offload()'
updates it to "RTE_ETHER_MTU + PMD specific Ethernet overhead" when it
is zero.
If '--max-pkt-len=N' argument provided, it will be used instead.
And with each "port config all max-pkt-len" command, the
'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag, 'max-pkt-len' and MTU is
updated.
[1]
--------------------------------------------------------------------------
dpdk-testpmd -c 0xf -n 4 -- -i --max-pkt-len=9000 --tx-offloads=0x8000
--rxq=4 --txq=4 --disable-rss
testpmd> set verbose 3
testpmd> port stop all
testpmd> port config all max-pkt-len 1518
testpmd> port start all
// Got fail error info without this patch
Configuring Port 0 (socket 1)
Ethdev port_id=0 rx_queue_id=0, new added offloads 0x800 must be
within per-queue offload capabilities 0x0 in rte_eth_rx_queue_setup()
Fail to configure port 0 rx queues //<-- Fail error info;
--------------------------------------------------------------------------
Fixes: 761c4d66900f ("app/testpmd: fix max Rx packet length for VLAN packets")
Cc: stable@dpdk.org
Signed-off-by: Steve Yang <stevex.yang@intel.com>
Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
v5:
* 'update_jumbo_frame_offload()' helper updated
* check zero 'max-pkt-len' value
* Update how queue offload flags updated
* Update MTU if JUMBO_FRAME flag is not set
* Default testpmd 'max-pkt-len' value set to zero
Cc: lance.richardson@broadcom.com
Cc: oulijun@huawei.com
Cc: wisamm@mellanox.com
Cc: lihuisong@huawei.com
---
app/test-pmd/cmdline.c | 13 ++++++
app/test-pmd/testpmd.c | 102 +++++++++++++++++++++++++++++++++--------
app/test-pmd/testpmd.h | 1 +
3 files changed, 97 insertions(+), 19 deletions(-)
Comments
On Mon, Jan 25, 2021 at 1:15 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>
> From: Steve Yang <stevex.yang@intel.com>
>
> "port config all max-pkt-len" command fails because it doesn't set the
> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag properly.
>
> Commit in the fixes line moved the 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload
> flag update from 'cmd_config_max_pkt_len_parsed()' to 'init_config()'.
> 'init_config()' function is only called during testpmd startup, but the
> flag status needs to be calculated whenever 'max_rx_pkt_len' changes.
>
> The issue can be reproduce as [1], where the 'max-pkt-len' reduced and
> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag should be cleared but it
> didn't.
>
> Adding the 'update_jumbo_frame_offload()' helper function to update
> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag and 'max_rx_pkt_len'. This
> function is called both by 'init_config()' and
> 'cmd_config_max_pkt_len_parsed()'.
>
> Default 'max-pkt-len' value set to zero, 'update_jumbo_frame_offload()'
> updates it to "RTE_ETHER_MTU + PMD specific Ethernet overhead" when it
> is zero.
> If '--max-pkt-len=N' argument provided, it will be used instead.
> And with each "port config all max-pkt-len" command, the
> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag, 'max-pkt-len' and MTU is
> updated.
>
> [1]
<snip>
> +/*
> + * Helper function to arrange max_rx_pktlen value and JUMBO_FRAME offload,
> + * MTU is also aligned if JUMBO_FRAME offload is not set.
> + *
> + * port->dev_info should be get before calling this function.
Should this be "port->dev_info should be set ..." instead?
<snip>
> + if (rx_offloads != port->dev_conf.rxmode.offloads) {
> + uint16_t qid;
> +
> + port->dev_conf.rxmode.offloads = rx_offloads;
> +
> + /* Apply JUMBO_FRAME offload configuration to Rx queue(s) */
> + for (qid = 0; qid < port->dev_info.nb_rx_queues; qid++) {
> + if (on)
> + port->rx_conf[qid].offloads |= DEV_RX_OFFLOAD_JUMBO_FRAME;
> + else
> + port->rx_conf[qid].offloads &= ~DEV_RX_OFFLOAD_JUMBO_FRAME;
> + }
Is it correct to set per-queue offloads that aren't advertised by the PMD
as supported in rx_queue_offload_capa?
> + }
> +
> + /* If JUMBO_FRAME is set MTU conversion done by ethdev layer,
> + * if unset do it here
> + */
> + if ((rx_offloads & DEV_RX_OFFLOAD_JUMBO_FRAME) == 0) {
> + ret = rte_eth_dev_set_mtu(portid,
> + port->dev_conf.rxmode.max_rx_pkt_len - eth_overhead);
> + if (ret)
> + printf("Failed to set MTU to %u for port %u\n",
> + port->dev_conf.rxmode.max_rx_pkt_len - eth_overhead,
> + portid);
> + }
> +
> + return 0;
> +}
> +
Applied and tested with a few iterations of configuring max packet size
back and forth between jumbo and non-jumbo sizes, also tried setting
max packet size using the command-line option, all seems good based
on rx offloads and packet forwarding.
Two minor questions above, otherwise LGTM.
On 1/25/2021 7:41 PM, Lance Richardson wrote:
> On Mon, Jan 25, 2021 at 1:15 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>>
>> From: Steve Yang <stevex.yang@intel.com>
>>
>> "port config all max-pkt-len" command fails because it doesn't set the
>> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag properly.
>>
>> Commit in the fixes line moved the 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload
>> flag update from 'cmd_config_max_pkt_len_parsed()' to 'init_config()'.
>> 'init_config()' function is only called during testpmd startup, but the
>> flag status needs to be calculated whenever 'max_rx_pkt_len' changes.
>>
>> The issue can be reproduce as [1], where the 'max-pkt-len' reduced and
>> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag should be cleared but it
>> didn't.
>>
>> Adding the 'update_jumbo_frame_offload()' helper function to update
>> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag and 'max_rx_pkt_len'. This
>> function is called both by 'init_config()' and
>> 'cmd_config_max_pkt_len_parsed()'.
>>
>> Default 'max-pkt-len' value set to zero, 'update_jumbo_frame_offload()'
>> updates it to "RTE_ETHER_MTU + PMD specific Ethernet overhead" when it
>> is zero.
>> If '--max-pkt-len=N' argument provided, it will be used instead.
>> And with each "port config all max-pkt-len" command, the
>> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag, 'max-pkt-len' and MTU is
>> updated.
>>
>> [1]
>
> <snip>
>
>> +/*
>> + * Helper function to arrange max_rx_pktlen value and JUMBO_FRAME offload,
>> + * MTU is also aligned if JUMBO_FRAME offload is not set.
>> + *
>> + * port->dev_info should be get before calling this function.
>
> Should this be "port->dev_info should be set ..." instead?
>
Ack.
>
> <snip>
>
>> + if (rx_offloads != port->dev_conf.rxmode.offloads) {
>> + uint16_t qid;
>> +
>> + port->dev_conf.rxmode.offloads = rx_offloads;
>> +
>> + /* Apply JUMBO_FRAME offload configuration to Rx queue(s) */
>> + for (qid = 0; qid < port->dev_info.nb_rx_queues; qid++) {
>> + if (on)
>> + port->rx_conf[qid].offloads |= DEV_RX_OFFLOAD_JUMBO_FRAME;
>> + else
>> + port->rx_conf[qid].offloads &= ~DEV_RX_OFFLOAD_JUMBO_FRAME;
>> + }
>
> Is it correct to set per-queue offloads that aren't advertised by the PMD
> as supported in rx_queue_offload_capa?
>
'port->rx_conf[]' is testpmd struct, and 'port->dev_conf.rxmode.offloads' values
are reflected to 'port->rx_conf[].offloads' for all queues.
We should set the offload in 'port->rx_conf[].offloads' if it is set in
'port->dev_conf.rxmode.offloads'.
If a port has capability for 'JUMBO_FRAME', 'port->rx_conf[].offloads' can have
it. And the port level capability is already checked above.
>> + }
>> +
>> + /* If JUMBO_FRAME is set MTU conversion done by ethdev layer,
>> + * if unset do it here
>> + */
>> + if ((rx_offloads & DEV_RX_OFFLOAD_JUMBO_FRAME) == 0) {
>> + ret = rte_eth_dev_set_mtu(portid,
>> + port->dev_conf.rxmode.max_rx_pkt_len - eth_overhead);
>> + if (ret)
>> + printf("Failed to set MTU to %u for port %u\n",
>> + port->dev_conf.rxmode.max_rx_pkt_len - eth_overhead,
>> + portid);
>> + }
>> +
>> + return 0;
>> +}
>> +
>
> Applied and tested with a few iterations of configuring max packet size
> back and forth between jumbo and non-jumbo sizes, also tried setting
> max packet size using the command-line option, all seems good based
> on rx offloads and packet forwarding.
>
> Two minor questions above, otherwise LGTM.
>
Thanks for testing. I will wait for more comments before new version.
Acked-by: Lance Richardson <lance.richardson@broadcom.com>
Thanks,
Lance
On Mon, Jan 25, 2021 at 7:44 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>
> On 1/25/2021 7:41 PM, Lance Richardson wrote:
> > On Mon, Jan 25, 2021 at 1:15 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> >>
> >> From: Steve Yang <stevex.yang@intel.com>
> >>
> >> "port config all max-pkt-len" command fails because it doesn't set the
> >> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag properly.
> >>
> >> Commit in the fixes line moved the 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload
> >> flag update from 'cmd_config_max_pkt_len_parsed()' to 'init_config()'.
> >> 'init_config()' function is only called during testpmd startup, but the
> >> flag status needs to be calculated whenever 'max_rx_pkt_len' changes.
> >>
> >> The issue can be reproduce as [1], where the 'max-pkt-len' reduced and
> >> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag should be cleared but it
> >> didn't.
> >>
> >> Adding the 'update_jumbo_frame_offload()' helper function to update
> >> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag and 'max_rx_pkt_len'. This
> >> function is called both by 'init_config()' and
> >> 'cmd_config_max_pkt_len_parsed()'.
> >>
> >> Default 'max-pkt-len' value set to zero, 'update_jumbo_frame_offload()'
> >> updates it to "RTE_ETHER_MTU + PMD specific Ethernet overhead" when it
> >> is zero.
> >> If '--max-pkt-len=N' argument provided, it will be used instead.
> >> And with each "port config all max-pkt-len" command, the
> >> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag, 'max-pkt-len' and MTU is
> >> updated.
> >>
> >> [1]
> >
> > <snip>
> >
> >> +/*
> >> + * Helper function to arrange max_rx_pktlen value and JUMBO_FRAME offload,
> >> + * MTU is also aligned if JUMBO_FRAME offload is not set.
> >> + *
> >> + * port->dev_info should be get before calling this function.
> >
> > Should this be "port->dev_info should be set ..." instead?
> >
>
> Ack.
>
> >
> > <snip>
> >
> >> + if (rx_offloads != port->dev_conf.rxmode.offloads) {
> >> + uint16_t qid;
> >> +
> >> + port->dev_conf.rxmode.offloads = rx_offloads;
> >> +
> >> + /* Apply JUMBO_FRAME offload configuration to Rx queue(s) */
> >> + for (qid = 0; qid < port->dev_info.nb_rx_queues; qid++) {
> >> + if (on)
> >> + port->rx_conf[qid].offloads |= DEV_RX_OFFLOAD_JUMBO_FRAME;
> >> + else
> >> + port->rx_conf[qid].offloads &= ~DEV_RX_OFFLOAD_JUMBO_FRAME;
> >> + }
> >
> > Is it correct to set per-queue offloads that aren't advertised by the PMD
> > as supported in rx_queue_offload_capa?
> >
>
> 'port->rx_conf[]' is testpmd struct, and 'port->dev_conf.rxmode.offloads' values
> are reflected to 'port->rx_conf[].offloads' for all queues.
>
> We should set the offload in 'port->rx_conf[].offloads' if it is set in
> 'port->dev_conf.rxmode.offloads'.
>
> If a port has capability for 'JUMBO_FRAME', 'port->rx_conf[].offloads' can have
> it. And the port level capability is already checked above.
>
> >> + }
> >> +
> >> + /* If JUMBO_FRAME is set MTU conversion done by ethdev layer,
> >> + * if unset do it here
> >> + */
> >> + if ((rx_offloads & DEV_RX_OFFLOAD_JUMBO_FRAME) == 0) {
> >> + ret = rte_eth_dev_set_mtu(portid,
> >> + port->dev_conf.rxmode.max_rx_pkt_len - eth_overhead);
> >> + if (ret)
> >> + printf("Failed to set MTU to %u for port %u\n",
> >> + port->dev_conf.rxmode.max_rx_pkt_len - eth_overhead,
> >> + portid);
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >
> > Applied and tested with a few iterations of configuring max packet size
> > back and forth between jumbo and non-jumbo sizes, also tried setting
> > max packet size using the command-line option, all seems good based
> > on rx offloads and packet forwarding.
> >
> > Two minor questions above, otherwise LGTM.
> >
>
> Thanks for testing. I will wait for more comments before new version.
On Mon, Jan 25, 2021 at 7:44 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>
> >> + if (rx_offloads != port->dev_conf.rxmode.offloads) {
> >> + uint16_t qid;
> >> +
> >> + port->dev_conf.rxmode.offloads = rx_offloads;
> >> +
> >> + /* Apply JUMBO_FRAME offload configuration to Rx queue(s) */
> >> + for (qid = 0; qid < port->dev_info.nb_rx_queues; qid++) {
> >> + if (on)
> >> + port->rx_conf[qid].offloads |= DEV_RX_OFFLOAD_JUMBO_FRAME;
> >> + else
> >> + port->rx_conf[qid].offloads &= ~DEV_RX_OFFLOAD_JUMBO_FRAME;
> >> + }
> >
> > Is it correct to set per-queue offloads that aren't advertised by the PMD
> > as supported in rx_queue_offload_capa?
> >
>
> 'port->rx_conf[]' is testpmd struct, and 'port->dev_conf.rxmode.offloads' values
> are reflected to 'port->rx_conf[].offloads' for all queues.
>
> We should set the offload in 'port->rx_conf[].offloads' if it is set in
> 'port->dev_conf.rxmode.offloads'.
>
> If a port has capability for 'JUMBO_FRAME', 'port->rx_conf[].offloads' can have
> it. And the port level capability is already checked above.
>
I'm still not 100% clear about the per-queue offload question.
With this patch, and jumbo max packet size configured (on the command
line in this case), I see:
testpmd> show port 0 rx_offload configuration
Rx Offloading Configuration of port 0 :
Port : JUMBO_FRAME
Queue[ 0] : JUMBO_FRAME
testpmd> show port 0 rx_offload capabilities
Rx Offloading Capabilities of port 0 :
Per Queue :
Per Port : VLAN_STRIP IPV4_CKSUM UDP_CKSUM TCP_CKSUM TCP_LRO
OUTER_IPV4_CKSUM VLAN_FILTER VLAN_EXTEND JUMBO_FRAME SCATTER TIMESTAMP
KEEP_CRC OUTER_UDP_CKSUM RSS_HASH
Yet if I configure a jumbo MTU starting with standard max packet size,
jumbo is only enabled at the port level:
testpmd> port config mtu 0 9000
testpmd> port start all
testpmd> show port 0 rx_offload configuration
Rx Offloading Configuration of port 0 :
Port : JUMBO_FRAME
Queue[ 0] :
It still seems odd for a per-queue offload to be enabled on a PMD that
doesn't support per-queue receive offloads.
> -----Original Message-----
> From: Lance Richardson <lance.richardson@broadcom.com>
> Sent: Tuesday, January 26, 2021 11:45
> To: Yigit, Ferruh <ferruh.yigit@intel.com>
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Li, Xiaoyun <xiaoyun.li@intel.com>;
> Iremonger, Bernard <bernard.iremonger@intel.com>; Yang, SteveX
> <stevex.yang@intel.com>; dev@dpdk.org; stable@dpdk.org;
> oulijun@huawei.com; wisamm@mellanox.com; lihuisong@huawei.com
> Subject: Re: [PATCH v5] app/testpmd: fix setting maximum packet length
>
> On Mon, Jan 25, 2021 at 7:44 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> >
> > >> + if (rx_offloads != port->dev_conf.rxmode.offloads) {
> > >> + uint16_t qid;
> > >> +
> > >> + port->dev_conf.rxmode.offloads = rx_offloads;
> > >> +
> > >> + /* Apply JUMBO_FRAME offload configuration to Rx queue(s) */
> > >> + for (qid = 0; qid < port->dev_info.nb_rx_queues; qid++) {
> > >> + if (on)
> > >> + port->rx_conf[qid].offloads |=
> DEV_RX_OFFLOAD_JUMBO_FRAME;
> > >> + else
> > >> + port->rx_conf[qid].offloads &=
> ~DEV_RX_OFFLOAD_JUMBO_FRAME;
> > >> + }
> > >
> > > Is it correct to set per-queue offloads that aren't advertised by the PMD
> > > as supported in rx_queue_offload_capa?
> > >
> >
> > 'port->rx_conf[]' is testpmd struct, and 'port->dev_conf.rxmode.offloads'
> values
> > are reflected to 'port->rx_conf[].offloads' for all queues.
> >
> > We should set the offload in 'port->rx_conf[].offloads' if it is set in
> > 'port->dev_conf.rxmode.offloads'.
> >
> > If a port has capability for 'JUMBO_FRAME', 'port->rx_conf[].offloads' can
> have
> > it. And the port level capability is already checked above.
> >
>
> I'm still not 100% clear about the per-queue offload question.
>
> With this patch, and jumbo max packet size configured (on the command
> line in this case), I see:
>
> testpmd> show port 0 rx_offload configuration
> Rx Offloading Configuration of port 0 :
> Port : JUMBO_FRAME
> Queue[ 0] : JUMBO_FRAME
>
> testpmd> show port 0 rx_offload capabilities
> Rx Offloading Capabilities of port 0 :
> Per Queue :
> Per Port : VLAN_STRIP IPV4_CKSUM UDP_CKSUM TCP_CKSUM TCP_LRO
> OUTER_IPV4_CKSUM VLAN_FILTER VLAN_EXTEND JUMBO_FRAME SCATTER
> TIMESTAMP
> KEEP_CRC OUTER_UDP_CKSUM RSS_HASH
>
> Yet if I configure a jumbo MTU starting with standard max packet size,
> jumbo is only enabled at the port level:
> testpmd> port config mtu 0 9000
> testpmd> port start all
>
> testpmd> show port 0 rx_offload configuration
> Rx Offloading Configuration of port 0 :
> Port : JUMBO_FRAME
> Queue[ 0] :
>
> It still seems odd for a per-queue offload to be enabled on a PMD that
> doesn't support per-queue receive offloads.
In struct rte_eth_dev_info, rx_offload_capa means All RX offload capabilities including all per-queue ones.
And rx_queue_offload_capa means Device per-queue RX offload capabilities.
The meaning of rx_queue_offload_capa is a bit of confusing between here and driver.
I think here rx_queue_offload_capa means whether a queue supports offloads.
But some drivers like i40e don't use rx_queue_offload_capa, set this as 0 and only use global rx_offload_capa.
I guess it's because the driver doesn't want to support different offloads settings for different queues?
Then rx_queue_offload_capa means differently.
Actually for i40e, it can support different offloads for different queues since there is 'offloads' in struct i40e_rx_queue.
I40e can just set rx_queue_offload_capa as the value for rx_offload_capa.
But maybe some drivers really don't want this? Not sure on this.
Hi,
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Monday, January 25, 2021 8:16 PM
> To: Wenzhuo Lu <wenzhuo.lu@intel.com>; Xiaoyun Li
> <xiaoyun.li@intel.com>; Bernard Iremonger
> <bernard.iremonger@intel.com>; Steve Yang <stevex.yang@intel.com>
> Cc: Ferruh Yigit <ferruh.yigit@intel.com>; dev@dpdk.org; stable@dpdk.org;
> lance.richardson@broadcom.com; oulijun@huawei.com; Wisam Monther
> <wisamm@nvidia.com>; lihuisong@huawei.com
> Subject: [PATCH v5] app/testpmd: fix setting maximum packet length
>
> From: Steve Yang <stevex.yang@intel.com>
>
> "port config all max-pkt-len" command fails because it doesn't set the
> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag properly.
>
> Commit in the fixes line moved the 'DEV_RX_OFFLOAD_JUMBO_FRAME'
> offload flag update from 'cmd_config_max_pkt_len_parsed()' to
> 'init_config()'.
> 'init_config()' function is only called during testpmd startup, but the flag
> status needs to be calculated whenever 'max_rx_pkt_len' changes.
>
> The issue can be reproduce as [1], where the 'max-pkt-len' reduced and
> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag should be cleared but it
> didn't.
>
> Adding the 'update_jumbo_frame_offload()' helper function to update
> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag and 'max_rx_pkt_len'. This
> function is called both by 'init_config()' and
> 'cmd_config_max_pkt_len_parsed()'.
>
> Default 'max-pkt-len' value set to zero, 'update_jumbo_frame_offload()'
> updates it to "RTE_ETHER_MTU + PMD specific Ethernet overhead" when it is
> zero.
> If '--max-pkt-len=N' argument provided, it will be used instead.
> And with each "port config all max-pkt-len" command, the
> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag, 'max-pkt-len' and MTU is
> updated.
>
> [1]
> --------------------------------------------------------------------------
> dpdk-testpmd -c 0xf -n 4 -- -i --max-pkt-len=9000 --tx-offloads=0x8000
> --rxq=4 --txq=4 --disable-rss
> testpmd> set verbose 3
> testpmd> port stop all
> testpmd> port config all max-pkt-len 1518 port start all
>
> // Got fail error info without this patch Configuring Port 0 (socket 1) Ethdev
> port_id=0 rx_queue_id=0, new added offloads 0x800 must be within per-
> queue offload capabilities 0x0 in rte_eth_rx_queue_setup() Fail to configure
> port 0 rx queues //<-- Fail error info;
> --------------------------------------------------------------------------
>
> Fixes: 761c4d66900f ("app/testpmd: fix max Rx packet length for VLAN
> packets")
> Cc: stable@dpdk.org
>
> Signed-off-by: Steve Yang <stevex.yang@intel.com>
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
>
> v5:
> * 'update_jumbo_frame_offload()' helper updated
> * check zero 'max-pkt-len' value
> * Update how queue offload flags updated
> * Update MTU if JUMBO_FRAME flag is not set
> * Default testpmd 'max-pkt-len' value set to zero
>
> Cc: lance.richardson@broadcom.com
> Cc: oulijun@huawei.com
> Cc: wisamm@mellanox.com
> Cc: lihuisong@huawei.com
> ---
It fixed this bug indeed: https://bugs.dpdk.org/show_bug.cgi?id=625
Thanks
Acked-by: Wisam Jaddo <wisamm@nvidia.com>
BRs,
Wisam Jaddo
On 1/26/2021 3:45 AM, Lance Richardson wrote:
> On Mon, Jan 25, 2021 at 7:44 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>>
>>>> + if (rx_offloads != port->dev_conf.rxmode.offloads) {
>>>> + uint16_t qid;
>>>> +
>>>> + port->dev_conf.rxmode.offloads = rx_offloads;
>>>> +
>>>> + /* Apply JUMBO_FRAME offload configuration to Rx queue(s) */
>>>> + for (qid = 0; qid < port->dev_info.nb_rx_queues; qid++) {
>>>> + if (on)
>>>> + port->rx_conf[qid].offloads |= DEV_RX_OFFLOAD_JUMBO_FRAME;
>>>> + else
>>>> + port->rx_conf[qid].offloads &= ~DEV_RX_OFFLOAD_JUMBO_FRAME;
>>>> + }
>>>
>>> Is it correct to set per-queue offloads that aren't advertised by the PMD
>>> as supported in rx_queue_offload_capa?
>>>
>>
>> 'port->rx_conf[]' is testpmd struct, and 'port->dev_conf.rxmode.offloads' values
>> are reflected to 'port->rx_conf[].offloads' for all queues.
>>
>> We should set the offload in 'port->rx_conf[].offloads' if it is set in
>> 'port->dev_conf.rxmode.offloads'.
>>
>> If a port has capability for 'JUMBO_FRAME', 'port->rx_conf[].offloads' can have
>> it. And the port level capability is already checked above.
>>
>
> I'm still not 100% clear about the per-queue offload question.
>
> With this patch, and jumbo max packet size configured (on the command
> line in this case), I see:
>
> testpmd> show port 0 rx_offload configuration
> Rx Offloading Configuration of port 0 :
> Port : JUMBO_FRAME
> Queue[ 0] : JUMBO_FRAME
>
> testpmd> show port 0 rx_offload capabilities
> Rx Offloading Capabilities of port 0 :
> Per Queue :
> Per Port : VLAN_STRIP IPV4_CKSUM UDP_CKSUM TCP_CKSUM TCP_LRO
> OUTER_IPV4_CKSUM VLAN_FILTER VLAN_EXTEND JUMBO_FRAME SCATTER TIMESTAMP
> KEEP_CRC OUTER_UDP_CKSUM RSS_HASH
>
The port level offload is applied to all queues on the port, testpmd config
structure reflects this logic in implementation.
If Rx offload X is set for a port, it is set for all Rx queue offloads, this is
not new behavior and not related to this patch.
In the ethdev, lets assume X & Y are port level offloads,
after X, Y are set via 'rte_eth_dev_configure()'
if user calls 'rte_eth_rx_queue_setup()' with X & Y offload, this is a valid
call and API will return success, since those offloads already enabled in port
level means they are enabled for all queues.
Because of above ethdev behavior, testpmd keeps all enabled port level offload
in the queue level offload too, and display them as enabled offloads for the queue.
To request a queue specific offload, it is added to specific queue's config
before calling queue setup. Lets say that queue specific offload is Z, after
setup testpmd config struct will show that specific queue has X, Y & Z offloads.
I hope it is more clear now.
> Yet if I configure a jumbo MTU starting with standard max packet size,
> jumbo is only enabled at the port level:
> testpmd> port config mtu 0 9000
> testpmd> port start all
>
> testpmd> show port 0 rx_offload configuration
> Rx Offloading Configuration of port 0 :
> Port : JUMBO_FRAME
> Queue[ 0] :
>
> It still seems odd for a per-queue offload to be enabled on a PMD that
> doesn't support per-queue receive offloads.
>
"port config mtu" should take queue offloads into account, it looks wrong right now.
Except a minor typo inline.
Acked-by: Xiaoyun Li <xiaoyun.li@intel.com>
> -----Original Message-----
> From: Yigit, Ferruh <ferruh.yigit@intel.com>
> Sent: Tuesday, January 26, 2021 02:16
> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Li, Xiaoyun <xiaoyun.li@intel.com>;
> Iremonger, Bernard <bernard.iremonger@intel.com>; Yang, SteveX
> <stevex.yang@intel.com>
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; dev@dpdk.org; stable@dpdk.org;
> lance.richardson@broadcom.com; oulijun@huawei.com;
> wisamm@mellanox.com; lihuisong@huawei.com
> Subject: [PATCH v5] app/testpmd: fix setting maximum packet length
>
> From: Steve Yang <stevex.yang@intel.com>
>
> "port config all max-pkt-len" command fails because it doesn't set the
> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag properly.
>
> Commit in the fixes line moved the 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload
> flag update from 'cmd_config_max_pkt_len_parsed()' to 'init_config()'.
> 'init_config()' function is only called during testpmd startup, but the flag status
> needs to be calculated whenever 'max_rx_pkt_len' changes.
>
> The issue can be reproduce as [1], where the 'max-pkt-len' reduced and
reproduced
> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag should be cleared but it didn't.
>
> Adding the 'update_jumbo_frame_offload()' helper function to update
> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag and 'max_rx_pkt_len'. This
> function is called both by 'init_config()' and 'cmd_config_max_pkt_len_parsed()'.
>
> Default 'max-pkt-len' value set to zero, 'update_jumbo_frame_offload()'
> updates it to "RTE_ETHER_MTU + PMD specific Ethernet overhead" when it is
> zero.
> If '--max-pkt-len=N' argument provided, it will be used instead.
> And with each "port config all max-pkt-len" command, the
> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag, 'max-pkt-len' and MTU is
> updated.
>
> [1]
> --------------------------------------------------------------------------
> dpdk-testpmd -c 0xf -n 4 -- -i --max-pkt-len=9000 --tx-offloads=0x8000
> --rxq=4 --txq=4 --disable-rss
> testpmd> set verbose 3
> testpmd> port stop all
> testpmd> port config all max-pkt-len 1518 port start all
>
> // Got fail error info without this patch Configuring Port 0 (socket 1) Ethdev
> port_id=0 rx_queue_id=0, new added offloads 0x800 must be within per-queue
> offload capabilities 0x0 in rte_eth_rx_queue_setup() Fail to configure port 0 rx
> queues //<-- Fail error info;
> --------------------------------------------------------------------------
>
> Fixes: 761c4d66900f ("app/testpmd: fix max Rx packet length for VLAN packets")
> Cc: stable@dpdk.org
>
> Signed-off-by: Steve Yang <stevex.yang@intel.com>
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
>
> v5:
> * 'update_jumbo_frame_offload()' helper updated
> * check zero 'max-pkt-len' value
> * Update how queue offload flags updated
> * Update MTU if JUMBO_FRAME flag is not set
> * Default testpmd 'max-pkt-len' value set to zero
>
> Cc: lance.richardson@broadcom.com
> Cc: oulijun@huawei.com
> Cc: wisamm@mellanox.com
> Cc: lihuisong@huawei.com
> ---
> app/test-pmd/cmdline.c | 13 ++++++
> app/test-pmd/testpmd.c | 102 +++++++++++++++++++++++++++++++++--------
> app/test-pmd/testpmd.h | 1 +
> 3 files changed, 97 insertions(+), 19 deletions(-)
>
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> 89034c8b7272..9ada4316c6c0 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -1877,7 +1877,9 @@ cmd_config_max_pkt_len_parsed(void
> *parsed_result,
> __rte_unused void *data)
> {
> struct cmd_config_max_pkt_len_result *res = parsed_result;
> + uint32_t max_rx_pkt_len_backup = 0;
> portid_t pid;
> + int ret;
>
> if (!all_ports_stopped()) {
> printf("Please stop all ports first\n"); @@ -1896,7 +1898,18 @@
> cmd_config_max_pkt_len_parsed(void *parsed_result,
> if (res->value == port-
> >dev_conf.rxmode.max_rx_pkt_len)
> return;
>
> + ret = eth_dev_info_get_print_err(pid, &port->dev_info);
> + if (ret != 0) {
> + printf("rte_eth_dev_info_get() failed for
> port %u\n",
> + pid);
> + return;
> + }
> +
> + max_rx_pkt_len_backup = port-
> >dev_conf.rxmode.max_rx_pkt_len;
> +
> port->dev_conf.rxmode.max_rx_pkt_len = res->value;
> + if (update_jumbo_frame_offload(pid) != 0)
> + port->dev_conf.rxmode.max_rx_pkt_len =
> max_rx_pkt_len_backup;
> } else {
> printf("Unknown parameter\n");
> return;
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> c256e719aea2..b69fcd3fde72 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -443,8 +443,11 @@ lcoreid_t latencystats_lcore_id = -1;
> * Ethernet device configuration.
> */
> struct rte_eth_rxmode rx_mode = {
> - .max_rx_pkt_len = RTE_ETHER_MAX_LEN,
> - /**< Default maximum frame length. */
> + /* Default maximum frame length.
> + * Zero is converted to "RTE_ETHER_MTU + PMD Ethernet overhead"
> + * in init_config().
> + */
> + .max_rx_pkt_len = 0,
> };
>
> struct rte_eth_txmode tx_mode = {
> @@ -1410,7 +1413,6 @@ init_config(void)
> struct rte_gro_param gro_param;
> uint32_t gso_types;
> uint16_t data_size;
> - uint16_t eth_overhead;
> bool warning = 0;
> int k;
> int ret;
> @@ -1447,22 +1449,10 @@ init_config(void)
> rte_exit(EXIT_FAILURE,
> "rte_eth_dev_info_get() failed\n");
>
> - /* Update the max_rx_pkt_len to have MTU as
> RTE_ETHER_MTU */
> - if (port->dev_info.max_mtu != UINT16_MAX &&
> - port->dev_info.max_rx_pktlen > port->dev_info.max_mtu)
> - eth_overhead = port->dev_info.max_rx_pktlen -
> - port->dev_info.max_mtu;
> - else
> - eth_overhead =
> - RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN;
> -
> - if (port->dev_conf.rxmode.max_rx_pkt_len <=
> - (uint32_t)(RTE_ETHER_MTU + eth_overhead))
> - port->dev_conf.rxmode.max_rx_pkt_len =
> - RTE_ETHER_MTU + eth_overhead;
> - else
> - port->dev_conf.rxmode.offloads |=
> - DEV_RX_OFFLOAD_JUMBO_FRAME;
> + ret = update_jumbo_frame_offload(pid);
> + if (ret != 0)
> + printf("Updating jumbo frame offload failed for
> port %u\n",
> + pid);
>
> if (!(port->dev_info.tx_offload_capa &
> DEV_TX_OFFLOAD_MBUF_FAST_FREE)) @@ -3358,6
> +3348,80 @@ rxtx_port_config(struct rte_port *port)
> }
> }
>
> +/*
> + * Helper function to arrange max_rx_pktlen value and JUMBO_FRAME
> +offload,
> + * MTU is also aligned if JUMBO_FRAME offload is not set.
> + *
> + * port->dev_info should be get before calling this function.
> + *
> + * return 0 on success, negative on error */ int
> +update_jumbo_frame_offload(portid_t portid) {
> + struct rte_port *port = &ports[portid];
> + uint32_t eth_overhead;
> + uint64_t rx_offloads;
> + int ret;
> + bool on;
> +
> + /* Update the max_rx_pkt_len to have MTU as RTE_ETHER_MTU */
> + if (port->dev_info.max_mtu != UINT16_MAX &&
> + port->dev_info.max_rx_pktlen > port->dev_info.max_mtu)
> + eth_overhead = port->dev_info.max_rx_pktlen -
> + port->dev_info.max_mtu;
> + else
> + eth_overhead = RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN;
> +
> + rx_offloads = port->dev_conf.rxmode.offloads;
> +
> + /* Default config value is 0 to use PMD specific overhead */
> + if (port->dev_conf.rxmode.max_rx_pkt_len == 0)
> + port->dev_conf.rxmode.max_rx_pkt_len = RTE_ETHER_MTU +
> eth_overhead;
> +
> + if (port->dev_conf.rxmode.max_rx_pkt_len <= RTE_ETHER_MTU +
> eth_overhead) {
> + rx_offloads &= ~DEV_RX_OFFLOAD_JUMBO_FRAME;
> + on = false;
> + } else {
> + if ((port->dev_info.rx_offload_capa &
> DEV_RX_OFFLOAD_JUMBO_FRAME) == 0) {
> + printf("Frame size (%u) is not supported by port %u\n",
> + port->dev_conf.rxmode.max_rx_pkt_len,
> + portid);
> + return -1;
> + }
> + rx_offloads |= DEV_RX_OFFLOAD_JUMBO_FRAME;
> + on = true;
> + }
> +
> + if (rx_offloads != port->dev_conf.rxmode.offloads) {
> + uint16_t qid;
> +
> + port->dev_conf.rxmode.offloads = rx_offloads;
> +
> + /* Apply JUMBO_FRAME offload configuration to Rx queue(s)
> */
> + for (qid = 0; qid < port->dev_info.nb_rx_queues; qid++) {
> + if (on)
> + port->rx_conf[qid].offloads |=
> DEV_RX_OFFLOAD_JUMBO_FRAME;
> + else
> + port->rx_conf[qid].offloads &=
> ~DEV_RX_OFFLOAD_JUMBO_FRAME;
> + }
> + }
> +
> + /* If JUMBO_FRAME is set MTU conversion done by ethdev layer,
> + * if unset do it here
> + */
> + if ((rx_offloads & DEV_RX_OFFLOAD_JUMBO_FRAME) == 0) {
> + ret = rte_eth_dev_set_mtu(portid,
> + port->dev_conf.rxmode.max_rx_pkt_len -
> eth_overhead);
> + if (ret)
> + printf("Failed to set MTU to %u for port %u\n",
> + port->dev_conf.rxmode.max_rx_pkt_len -
> eth_overhead,
> + portid);
> + }
> +
> + return 0;
> +}
> +
> void
> init_port_config(void)
> {
> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h index
> 5f2316210726..2f8f5a92e46a 100644
> --- a/app/test-pmd/testpmd.h
> +++ b/app/test-pmd/testpmd.h
> @@ -1005,6 +1005,7 @@ uint16_t tx_pkt_set_dynf(uint16_t port_id,
> __rte_unused uint16_t queue,
> __rte_unused void *user_param);
> void add_tx_dynf_callback(portid_t portid); void
> remove_tx_dynf_callback(portid_t portid);
> +int update_jumbo_frame_offload(portid_t portid);
>
> /*
> * Work-around of a compilation error with ICC on invocations of the
> --
> 2.29.2
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Li, Xiaoyun
> Sent: January 27, 2021 11:05
> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>; Iremonger, Bernard
> <bernard.iremonger@intel.com>; Yang, SteveX <stevex.yang@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org; lance.richardson@broadcom.com;
> oulijun@huawei.com; wisamm@mellanox.com; lihuisong@huawei.com
> Subject: Re: [dpdk-dev] [PATCH v5] app/testpmd: fix setting maximum
> packet length
>
> Except a minor typo inline.
> Acked-by: Xiaoyun Li <xiaoyun.li@intel.com>
>
Tested-by: Chen, BoX C <box.c.chen@intel.com>
Hi Ferruh,
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Monday, January 25, 2021 8:16 PM
> To: Wenzhuo Lu <wenzhuo.lu@intel.com>; Xiaoyun Li
> <xiaoyun.li@intel.com>; Bernard Iremonger
> <bernard.iremonger@intel.com>; Steve Yang <stevex.yang@intel.com>
> Cc: Ferruh Yigit <ferruh.yigit@intel.com>; dev@dpdk.org; stable@dpdk.org;
> lance.richardson@broadcom.com; oulijun@huawei.com; Wisam Monther
> <wisamm@nvidia.com>; lihuisong@huawei.com
> Subject: [PATCH v5] app/testpmd: fix setting maximum packet length
>
> From: Steve Yang <stevex.yang@intel.com>
>
> "port config all max-pkt-len" command fails because it doesn't set the
> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag properly.
>
> Commit in the fixes line moved the 'DEV_RX_OFFLOAD_JUMBO_FRAME'
> offload flag update from 'cmd_config_max_pkt_len_parsed()' to
> 'init_config()'.
> 'init_config()' function is only called during testpmd startup, but the flag
> status needs to be calculated whenever 'max_rx_pkt_len' changes.
>
> The issue can be reproduce as [1], where the 'max-pkt-len' reduced and
> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag should be cleared but it
> didn't.
>
> Adding the 'update_jumbo_frame_offload()' helper function to update
> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag and 'max_rx_pkt_len'. This
> function is called both by 'init_config()' and
> 'cmd_config_max_pkt_len_parsed()'.
>
> Default 'max-pkt-len' value set to zero, 'update_jumbo_frame_offload()'
> updates it to "RTE_ETHER_MTU + PMD specific Ethernet overhead" when it is
> zero.
> If '--max-pkt-len=N' argument provided, it will be used instead.
> And with each "port config all max-pkt-len" command, the
> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag, 'max-pkt-len' and MTU is
> updated.
>
> [1]
> --------------------------------------------------------------------------
> dpdk-testpmd -c 0xf -n 4 -- -i --max-pkt-len=9000 --tx-offloads=0x8000
> --rxq=4 --txq=4 --disable-rss
> testpmd> set verbose 3
> testpmd> port stop all
> testpmd> port config all max-pkt-len 1518 port start all
>
> // Got fail error info without this patch Configuring Port 0 (socket 1) Ethdev
> port_id=0 rx_queue_id=0, new added offloads 0x800 must be within per-
> queue offload capabilities 0x0 in rte_eth_rx_queue_setup() Fail to configure
> port 0 rx queues //<-- Fail error info;
> --------------------------------------------------------------------------
>
> Fixes: 761c4d66900f ("app/testpmd: fix max Rx packet length for VLAN
> packets")
> Cc: stable@dpdk.org
>
> Signed-off-by: Steve Yang <stevex.yang@intel.com>
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
>
> v5:
> * 'update_jumbo_frame_offload()' helper updated
> * check zero 'max-pkt-len' value
> * Update how queue offload flags updated
> * Update MTU if JUMBO_FRAME flag is not set
> * Default testpmd 'max-pkt-len' value set to zero
>
> Cc: lance.richardson@broadcom.com
> Cc: oulijun@huawei.com
> Cc: wisamm@mellanox.com
> Cc: lihuisong@huawei.com
> ---
I think we need to have https://bugs.dpdk.org/show_bug.cgi?id=625 ID in the commit log as fix,
In order to allow the scripts to close related bugs directly from Bugzilla.
BRs,
Wisam Jaddo
On 1/28/2021 9:18 AM, Wisam Monther wrote:
> Hi Ferruh,
>
>
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: Monday, January 25, 2021 8:16 PM
>> To: Wenzhuo Lu <wenzhuo.lu@intel.com>; Xiaoyun Li
>> <xiaoyun.li@intel.com>; Bernard Iremonger
>> <bernard.iremonger@intel.com>; Steve Yang <stevex.yang@intel.com>
>> Cc: Ferruh Yigit <ferruh.yigit@intel.com>; dev@dpdk.org; stable@dpdk.org;
>> lance.richardson@broadcom.com; oulijun@huawei.com; Wisam Monther
>> <wisamm@nvidia.com>; lihuisong@huawei.com
>> Subject: [PATCH v5] app/testpmd: fix setting maximum packet length
>>
>> From: Steve Yang <stevex.yang@intel.com>
>>
>> "port config all max-pkt-len" command fails because it doesn't set the
>> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag properly.
>>
>> Commit in the fixes line moved the 'DEV_RX_OFFLOAD_JUMBO_FRAME'
>> offload flag update from 'cmd_config_max_pkt_len_parsed()' to
>> 'init_config()'.
>> 'init_config()' function is only called during testpmd startup, but the flag
>> status needs to be calculated whenever 'max_rx_pkt_len' changes.
>>
>> The issue can be reproduce as [1], where the 'max-pkt-len' reduced and
>> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag should be cleared but it
>> didn't.
>>
>> Adding the 'update_jumbo_frame_offload()' helper function to update
>> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag and 'max_rx_pkt_len'. This
>> function is called both by 'init_config()' and
>> 'cmd_config_max_pkt_len_parsed()'.
>>
>> Default 'max-pkt-len' value set to zero, 'update_jumbo_frame_offload()'
>> updates it to "RTE_ETHER_MTU + PMD specific Ethernet overhead" when it is
>> zero.
>> If '--max-pkt-len=N' argument provided, it will be used instead.
>> And with each "port config all max-pkt-len" command, the
>> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag, 'max-pkt-len' and MTU is
>> updated.
>>
>> [1]
>> --------------------------------------------------------------------------
>> dpdk-testpmd -c 0xf -n 4 -- -i --max-pkt-len=9000 --tx-offloads=0x8000
>> --rxq=4 --txq=4 --disable-rss
>> testpmd> set verbose 3
>> testpmd> port stop all
>> testpmd> port config all max-pkt-len 1518 port start all
>>
>> // Got fail error info without this patch Configuring Port 0 (socket 1) Ethdev
>> port_id=0 rx_queue_id=0, new added offloads 0x800 must be within per-
>> queue offload capabilities 0x0 in rte_eth_rx_queue_setup() Fail to configure
>> port 0 rx queues //<-- Fail error info;
>> --------------------------------------------------------------------------
>>
>> Fixes: 761c4d66900f ("app/testpmd: fix max Rx packet length for VLAN
>> packets")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Steve Yang <stevex.yang@intel.com>
>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>> ---
>>
>> v5:
>> * 'update_jumbo_frame_offload()' helper updated
>> * check zero 'max-pkt-len' value
>> * Update how queue offload flags updated
>> * Update MTU if JUMBO_FRAME flag is not set
>> * Default testpmd 'max-pkt-len' value set to zero
>>
>> Cc: lance.richardson@broadcom.com
>> Cc: oulijun@huawei.com
>> Cc: wisamm@mellanox.com
>> Cc: lihuisong@huawei.com
>> ---
>
> I think we need to have https://bugs.dpdk.org/show_bug.cgi?id=625 ID in the commit log as fix,
Sure, I will send a new version with suggested updates, most probably today, I
can add the Bugzilla information too.
> In order to allow the scripts to close related bugs directly from Bugzilla.
>
Scripts? Do we have scripts that close defects automatically, I wasn't aware of
it, where does it run?
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Thursday, January 28, 2021 11:27 AM
> To: Wisam Monther <wisamm@nvidia.com>; Wenzhuo Lu
> <wenzhuo.lu@intel.com>; Xiaoyun Li <xiaoyun.li@intel.com>; Bernard
> Iremonger <bernard.iremonger@intel.com>; Steve Yang
> <stevex.yang@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org; lance.richardson@broadcom.com;
> oulijun@huawei.com; lihuisong@huawei.com
> Subject: Re: [PATCH v5] app/testpmd: fix setting maximum packet length
>
> On 1/28/2021 9:18 AM, Wisam Monther wrote:
> > Hi Ferruh,
> >
> >
> >> -----Original Message-----
> >> From: Ferruh Yigit <ferruh.yigit@intel.com>
> >> Sent: Monday, January 25, 2021 8:16 PM
> >> To: Wenzhuo Lu <wenzhuo.lu@intel.com>; Xiaoyun Li
> >> <xiaoyun.li@intel.com>; Bernard Iremonger
> >> <bernard.iremonger@intel.com>; Steve Yang <stevex.yang@intel.com>
> >> Cc: Ferruh Yigit <ferruh.yigit@intel.com>; dev@dpdk.org;
> >> stable@dpdk.org; lance.richardson@broadcom.com;
> oulijun@huawei.com;
> >> Wisam Monther <wisamm@nvidia.com>; lihuisong@huawei.com
> >> Subject: [PATCH v5] app/testpmd: fix setting maximum packet length
> >>
> >> From: Steve Yang <stevex.yang@intel.com>
> >>
> >> "port config all max-pkt-len" command fails because it doesn't set
> >> the 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag properly.
> >>
> >> Commit in the fixes line moved the 'DEV_RX_OFFLOAD_JUMBO_FRAME'
> >> offload flag update from 'cmd_config_max_pkt_len_parsed()' to
> >> 'init_config()'.
> >> 'init_config()' function is only called during testpmd startup, but
> >> the flag status needs to be calculated whenever 'max_rx_pkt_len'
> changes.
> >>
> >> The issue can be reproduce as [1], where the 'max-pkt-len' reduced
> >> and 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag should be cleared
> but
> >> it didn't.
> >>
> >> Adding the 'update_jumbo_frame_offload()' helper function to update
> >> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag and 'max_rx_pkt_len'.
> This
> >> function is called both by 'init_config()' and
> >> 'cmd_config_max_pkt_len_parsed()'.
> >>
> >> Default 'max-pkt-len' value set to zero, 'update_jumbo_frame_offload()'
> >> updates it to "RTE_ETHER_MTU + PMD specific Ethernet overhead" when
> >> it is zero.
> >> If '--max-pkt-len=N' argument provided, it will be used instead.
> >> And with each "port config all max-pkt-len" command, the
> >> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag, 'max-pkt-len' and MTU
> is
> >> updated.
> >>
> >> [1]
> >> ---------------------------------------------------------------------
> >> ----- dpdk-testpmd -c 0xf -n 4 -- -i --max-pkt-len=9000
> >> --tx-offloads=0x8000
> >> --rxq=4 --txq=4 --disable-rss
> >> testpmd> set verbose 3
> >> testpmd> port stop all
> >> testpmd> port config all max-pkt-len 1518 port start all
> >>
> >> // Got fail error info without this patch Configuring Port 0 (socket
> >> 1) Ethdev
> >> port_id=0 rx_queue_id=0, new added offloads 0x800 must be within per-
> >> queue offload capabilities 0x0 in rte_eth_rx_queue_setup() Fail to
> >> configure port 0 rx queues //<-- Fail error info;
> >> ---------------------------------------------------------------------
> >> -----
> >>
> >> Fixes: 761c4d66900f ("app/testpmd: fix max Rx packet length for VLAN
> >> packets")
> >> Cc: stable@dpdk.org
> >>
> >> Signed-off-by: Steve Yang <stevex.yang@intel.com>
> >> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> >> ---
> >>
> >> v5:
> >> * 'update_jumbo_frame_offload()' helper updated
> >> * check zero 'max-pkt-len' value
> >> * Update how queue offload flags updated
> >> * Update MTU if JUMBO_FRAME flag is not set
> >> * Default testpmd 'max-pkt-len' value set to zero
> >>
> >> Cc: lance.richardson@broadcom.com
> >> Cc: oulijun@huawei.com
> >> Cc: wisamm@mellanox.com
> >> Cc: lihuisong@huawei.com
> >> ---
> >
> > I think we need to have https://bugs.dpdk.org/show_bug.cgi?id=625 ID
> > in the commit log as fix,
>
>
> Sure, I will send a new version with suggested updates, most probably today,
> I can add the Bugzilla information too.
>
> > In order to allow the scripts to close related bugs directly from Bugzilla.
> >
>
> Scripts? Do we have scripts that close defects automatically, I wasn't aware of
> it, where does it run?
It's something Thomas using and run it every time to time, it's not published yet.
But it closes merged bugs, adding a link to the commit as comment.
On Tue, Jan 26, 2021 at 6:01 AM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>
> On 1/26/2021 3:45 AM, Lance Richardson wrote:
> > On Mon, Jan 25, 2021 at 7:44 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> >>
> >>>> + if (rx_offloads != port->dev_conf.rxmode.offloads) {
> >>>> + uint16_t qid;
> >>>> +
> >>>> + port->dev_conf.rxmode.offloads = rx_offloads;
> >>>> +
> >>>> + /* Apply JUMBO_FRAME offload configuration to Rx queue(s) */
> >>>> + for (qid = 0; qid < port->dev_info.nb_rx_queues; qid++) {
> >>>> + if (on)
> >>>> + port->rx_conf[qid].offloads |= DEV_RX_OFFLOAD_JUMBO_FRAME;
> >>>> + else
> >>>> + port->rx_conf[qid].offloads &= ~DEV_RX_OFFLOAD_JUMBO_FRAME;
> >>>> + }
> >>>
> >>> Is it correct to set per-queue offloads that aren't advertised by the PMD
> >>> as supported in rx_queue_offload_capa?
> >>>
> >>
> >> 'port->rx_conf[]' is testpmd struct, and 'port->dev_conf.rxmode.offloads' values
> >> are reflected to 'port->rx_conf[].offloads' for all queues.
> >>
> >> We should set the offload in 'port->rx_conf[].offloads' if it is set in
> >> 'port->dev_conf.rxmode.offloads'.
> >>
> >> If a port has capability for 'JUMBO_FRAME', 'port->rx_conf[].offloads' can have
> >> it. And the port level capability is already checked above.
> >>
> >
> > I'm still not 100% clear about the per-queue offload question.
> >
> > With this patch, and jumbo max packet size configured (on the command
> > line in this case), I see:
> >
> > testpmd> show port 0 rx_offload configuration
> > Rx Offloading Configuration of port 0 :
> > Port : JUMBO_FRAME
> > Queue[ 0] : JUMBO_FRAME
> >
> > testpmd> show port 0 rx_offload capabilities
> > Rx Offloading Capabilities of port 0 :
> > Per Queue :
> > Per Port : VLAN_STRIP IPV4_CKSUM UDP_CKSUM TCP_CKSUM TCP_LRO
> > OUTER_IPV4_CKSUM VLAN_FILTER VLAN_EXTEND JUMBO_FRAME SCATTER TIMESTAMP
> > KEEP_CRC OUTER_UDP_CKSUM RSS_HASH
> >
>
> The port level offload is applied to all queues on the port, testpmd config
> structure reflects this logic in implementation.
> If Rx offload X is set for a port, it is set for all Rx queue offloads, this is
> not new behavior and not related to this patch.
>
OK, is this purely for display purposes within testpmd? I ask because
it appears that all PMDs supporting per-queue offload configuration already
take care of combining port-level and per-queue offloads within their
tx_queue_setup()/rx_queue_setup() functions and then track the combined
set of offloads within a per-queue field, e.g. this line is common to
e1000/i40e/ionic/ixgbe/octeontx2/thunderx/txgbe rx_queue_setup()
implementations:
offloads = rx_conf->offloads | dev->data->dev_conf.rxmode.offloads;
And rte_ethdev.h says:
No need to repeat any bit in rx_conf->offloads which has already been
enabled in rte_eth_dev_configure() at port level. An offloading enabled
at port level can't be disabled at queue level.
Which I suppose confirms that if testpmd is combining per-port and per-
queue offloads, it's just for the purposes of testpmd.
Apologies for worrying at this even more, I just wanted to be sure that
I understand what the PMD is expected to do.
Regards,
Lance
On 1/28/2021 9:36 PM, Lance Richardson wrote:
> On Tue, Jan 26, 2021 at 6:01 AM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>>
>> On 1/26/2021 3:45 AM, Lance Richardson wrote:
>>> On Mon, Jan 25, 2021 at 7:44 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>>>>
>>>>>> + if (rx_offloads != port->dev_conf.rxmode.offloads) {
>>>>>> + uint16_t qid;
>>>>>> +
>>>>>> + port->dev_conf.rxmode.offloads = rx_offloads;
>>>>>> +
>>>>>> + /* Apply JUMBO_FRAME offload configuration to Rx queue(s) */
>>>>>> + for (qid = 0; qid < port->dev_info.nb_rx_queues; qid++) {
>>>>>> + if (on)
>>>>>> + port->rx_conf[qid].offloads |= DEV_RX_OFFLOAD_JUMBO_FRAME;
>>>>>> + else
>>>>>> + port->rx_conf[qid].offloads &= ~DEV_RX_OFFLOAD_JUMBO_FRAME;
>>>>>> + }
>>>>>
>>>>> Is it correct to set per-queue offloads that aren't advertised by the PMD
>>>>> as supported in rx_queue_offload_capa?
>>>>>
>>>>
>>>> 'port->rx_conf[]' is testpmd struct, and 'port->dev_conf.rxmode.offloads' values
>>>> are reflected to 'port->rx_conf[].offloads' for all queues.
>>>>
>>>> We should set the offload in 'port->rx_conf[].offloads' if it is set in
>>>> 'port->dev_conf.rxmode.offloads'.
>>>>
>>>> If a port has capability for 'JUMBO_FRAME', 'port->rx_conf[].offloads' can have
>>>> it. And the port level capability is already checked above.
>>>>
>>>
>>> I'm still not 100% clear about the per-queue offload question.
>>>
>>> With this patch, and jumbo max packet size configured (on the command
>>> line in this case), I see:
>>>
>>> testpmd> show port 0 rx_offload configuration
>>> Rx Offloading Configuration of port 0 :
>>> Port : JUMBO_FRAME
>>> Queue[ 0] : JUMBO_FRAME
>>>
>>> testpmd> show port 0 rx_offload capabilities
>>> Rx Offloading Capabilities of port 0 :
>>> Per Queue :
>>> Per Port : VLAN_STRIP IPV4_CKSUM UDP_CKSUM TCP_CKSUM TCP_LRO
>>> OUTER_IPV4_CKSUM VLAN_FILTER VLAN_EXTEND JUMBO_FRAME SCATTER TIMESTAMP
>>> KEEP_CRC OUTER_UDP_CKSUM RSS_HASH
>>>
>>
>> The port level offload is applied to all queues on the port, testpmd config
>> structure reflects this logic in implementation.
>> If Rx offload X is set for a port, it is set for all Rx queue offloads, this is
>> not new behavior and not related to this patch.
>>
> OK, is this purely for display purposes within testpmd? I ask because
> it appears that all PMDs supporting per-queue offload configuration already
> take care of combining port-level and per-queue offloads within their
> tx_queue_setup()/rx_queue_setup() functions and then track the combined
> set of offloads within a per-queue field, e.g. this line is common to
> e1000/i40e/ionic/ixgbe/octeontx2/thunderx/txgbe rx_queue_setup()
> implementations:
> offloads = rx_conf->offloads | dev->data->dev_conf.rxmode.offloads;
>
> And rte_ethdev.h says:
> No need to repeat any bit in rx_conf->offloads which has already been
> enabled in rte_eth_dev_configure() at port level. An offloading enabled
> at port level can't be disabled at queue level.
>
> Which I suppose confirms that if testpmd is combining per-port and per-
> queue offloads, it's just for the purposes of testpmd.
>
Yes it is just for purposed of testpmd (application), but doesn't need to be
just limited to display purpose, testpmd keeps the config locally for any need.
> Apologies for worrying at this even more, I just wanted to be sure that
> I understand what the PMD is expected to do.
>
PMDs should not be getting these offloads in queue setup, since ethdev layer
strips the port level offloads, I mean:
if port level offloads: X, Y
And applications provide following for queue_setup():
a) X
b) Y
c) X, Y
For all three PMD receives empty queue offload request.
d) X, Y, Z
PMD gets Z if it is in drivers queue specific capabilities.
@@ -1877,7 +1877,9 @@ cmd_config_max_pkt_len_parsed(void *parsed_result,
__rte_unused void *data)
{
struct cmd_config_max_pkt_len_result *res = parsed_result;
+ uint32_t max_rx_pkt_len_backup = 0;
portid_t pid;
+ int ret;
if (!all_ports_stopped()) {
printf("Please stop all ports first\n");
@@ -1896,7 +1898,18 @@ cmd_config_max_pkt_len_parsed(void *parsed_result,
if (res->value == port->dev_conf.rxmode.max_rx_pkt_len)
return;
+ ret = eth_dev_info_get_print_err(pid, &port->dev_info);
+ if (ret != 0) {
+ printf("rte_eth_dev_info_get() failed for port %u\n",
+ pid);
+ return;
+ }
+
+ max_rx_pkt_len_backup = port->dev_conf.rxmode.max_rx_pkt_len;
+
port->dev_conf.rxmode.max_rx_pkt_len = res->value;
+ if (update_jumbo_frame_offload(pid) != 0)
+ port->dev_conf.rxmode.max_rx_pkt_len = max_rx_pkt_len_backup;
} else {
printf("Unknown parameter\n");
return;
@@ -443,8 +443,11 @@ lcoreid_t latencystats_lcore_id = -1;
* Ethernet device configuration.
*/
struct rte_eth_rxmode rx_mode = {
- .max_rx_pkt_len = RTE_ETHER_MAX_LEN,
- /**< Default maximum frame length. */
+ /* Default maximum frame length.
+ * Zero is converted to "RTE_ETHER_MTU + PMD Ethernet overhead"
+ * in init_config().
+ */
+ .max_rx_pkt_len = 0,
};
struct rte_eth_txmode tx_mode = {
@@ -1410,7 +1413,6 @@ init_config(void)
struct rte_gro_param gro_param;
uint32_t gso_types;
uint16_t data_size;
- uint16_t eth_overhead;
bool warning = 0;
int k;
int ret;
@@ -1447,22 +1449,10 @@ init_config(void)
rte_exit(EXIT_FAILURE,
"rte_eth_dev_info_get() failed\n");
- /* Update the max_rx_pkt_len to have MTU as RTE_ETHER_MTU */
- if (port->dev_info.max_mtu != UINT16_MAX &&
- port->dev_info.max_rx_pktlen > port->dev_info.max_mtu)
- eth_overhead = port->dev_info.max_rx_pktlen -
- port->dev_info.max_mtu;
- else
- eth_overhead =
- RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN;
-
- if (port->dev_conf.rxmode.max_rx_pkt_len <=
- (uint32_t)(RTE_ETHER_MTU + eth_overhead))
- port->dev_conf.rxmode.max_rx_pkt_len =
- RTE_ETHER_MTU + eth_overhead;
- else
- port->dev_conf.rxmode.offloads |=
- DEV_RX_OFFLOAD_JUMBO_FRAME;
+ ret = update_jumbo_frame_offload(pid);
+ if (ret != 0)
+ printf("Updating jumbo frame offload failed for port %u\n",
+ pid);
if (!(port->dev_info.tx_offload_capa &
DEV_TX_OFFLOAD_MBUF_FAST_FREE))
@@ -3358,6 +3348,80 @@ rxtx_port_config(struct rte_port *port)
}
}
+/*
+ * Helper function to arrange max_rx_pktlen value and JUMBO_FRAME offload,
+ * MTU is also aligned if JUMBO_FRAME offload is not set.
+ *
+ * port->dev_info should be get before calling this function.
+ *
+ * return 0 on success, negative on error
+ */
+int
+update_jumbo_frame_offload(portid_t portid)
+{
+ struct rte_port *port = &ports[portid];
+ uint32_t eth_overhead;
+ uint64_t rx_offloads;
+ int ret;
+ bool on;
+
+ /* Update the max_rx_pkt_len to have MTU as RTE_ETHER_MTU */
+ if (port->dev_info.max_mtu != UINT16_MAX &&
+ port->dev_info.max_rx_pktlen > port->dev_info.max_mtu)
+ eth_overhead = port->dev_info.max_rx_pktlen -
+ port->dev_info.max_mtu;
+ else
+ eth_overhead = RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN;
+
+ rx_offloads = port->dev_conf.rxmode.offloads;
+
+ /* Default config value is 0 to use PMD specific overhead */
+ if (port->dev_conf.rxmode.max_rx_pkt_len == 0)
+ port->dev_conf.rxmode.max_rx_pkt_len = RTE_ETHER_MTU + eth_overhead;
+
+ if (port->dev_conf.rxmode.max_rx_pkt_len <= RTE_ETHER_MTU + eth_overhead) {
+ rx_offloads &= ~DEV_RX_OFFLOAD_JUMBO_FRAME;
+ on = false;
+ } else {
+ if ((port->dev_info.rx_offload_capa & DEV_RX_OFFLOAD_JUMBO_FRAME) == 0) {
+ printf("Frame size (%u) is not supported by port %u\n",
+ port->dev_conf.rxmode.max_rx_pkt_len,
+ portid);
+ return -1;
+ }
+ rx_offloads |= DEV_RX_OFFLOAD_JUMBO_FRAME;
+ on = true;
+ }
+
+ if (rx_offloads != port->dev_conf.rxmode.offloads) {
+ uint16_t qid;
+
+ port->dev_conf.rxmode.offloads = rx_offloads;
+
+ /* Apply JUMBO_FRAME offload configuration to Rx queue(s) */
+ for (qid = 0; qid < port->dev_info.nb_rx_queues; qid++) {
+ if (on)
+ port->rx_conf[qid].offloads |= DEV_RX_OFFLOAD_JUMBO_FRAME;
+ else
+ port->rx_conf[qid].offloads &= ~DEV_RX_OFFLOAD_JUMBO_FRAME;
+ }
+ }
+
+ /* If JUMBO_FRAME is set MTU conversion done by ethdev layer,
+ * if unset do it here
+ */
+ if ((rx_offloads & DEV_RX_OFFLOAD_JUMBO_FRAME) == 0) {
+ ret = rte_eth_dev_set_mtu(portid,
+ port->dev_conf.rxmode.max_rx_pkt_len - eth_overhead);
+ if (ret)
+ printf("Failed to set MTU to %u for port %u\n",
+ port->dev_conf.rxmode.max_rx_pkt_len - eth_overhead,
+ portid);
+ }
+
+ return 0;
+}
+
void
init_port_config(void)
{
@@ -1005,6 +1005,7 @@ uint16_t tx_pkt_set_dynf(uint16_t port_id, __rte_unused uint16_t queue,
__rte_unused void *user_param);
void add_tx_dynf_callback(portid_t portid);
void remove_tx_dynf_callback(portid_t portid);
+int update_jumbo_frame_offload(portid_t portid);
/*
* Work-around of a compilation error with ICC on invocations of the