diff mbox series

[v2] net/ice: fix setting max frame size

Message ID 20191028155453.37207-1-jiaqix.min@intel.com (mailing list archive)
State Superseded, archived
Delegated to: xiaolong ye
Headers
Series [v2] net/ice: fix setting max frame size |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-compilation success Compile Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/travis-robot success Travis build: passed
ci/Intel-compilation fail Compilation issues

Commit Message

Min, JiaqiX Oct. 28, 2019, 3:54 p.m. UTC 
Max frame size is not set to HW, so packets above the MTU
do not get dropped by HW. The patch fixed the issue.

Fixes: ae2bdd0219cb ("net/ice: support MTU setting")
Cc: stable@dpdk.org

Signed-off-by: Min JiaqiX <jiaqix.min@intel.com>
---
v2:
* Changed commit message.
---
 drivers/net/ice/ice_ethdev.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Xiaolong Ye Oct. 28, 2019, 8:46 a.m. UTC | #1 
On 10/28, Min JiaqiX wrote:
>Max frame size is not set to HW, so packets above the MTU
>do not get dropped by HW. The patch fixed the issue.
>
>Fixes: ae2bdd0219cb ("net/ice: support MTU setting")

We should set max frame size to HW in the beginning when the driver was introduced, so
the fix commit should be 50370662b727 ("net/ice: support device and queue ops")

Fix this while merging.

>Cc: stable@dpdk.org
>
>Signed-off-by: Min JiaqiX <jiaqix.min@intel.com>
>---
>v2:
>* Changed commit message.
>---
> drivers/net/ice/ice_ethdev.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
>diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
>index 022b58c01..403305cfb 100644
>--- a/drivers/net/ice/ice_ethdev.c
>+++ b/drivers/net/ice/ice_ethdev.c
>@@ -2506,6 +2506,10 @@ ice_dev_start(struct rte_eth_dev *dev)
> 
> 	pf->adapter_stopped = false;
> 
>+	/* Set the max frame size */
>+	ice_aq_set_mac_cfg(hw,
>+		pf->dev_data->dev_conf.rxmode.max_rx_pkt_len, NULL);
>+

Why set the max frame size to HW in start ops, not in init stage like in i40e?
and what if max_rx_pkt_len hasn't been set? 


Thanks,
Xiaolong


> 	return 0;
> 
> 	/* stop the started queues if failed to start all queues */
>-- 
>2.17.1
>

Reviewed-by: Xiaolong Ye <xiaolong.ye@intel.com>

Applied to dpdk-next-net-intel. Thanks.
Xiaolong Ye Oct. 29, 2019, 2:05 a.m. UTC | #2 
On 10/28, Ye Xiaolong wrote:

>Reviewed-by: Xiaolong Ye <xiaolong.ye@intel.com>
>
>Applied to dpdk-next-net-intel. Thanks.

Sorry for the above misleading message, this patch is still under discussion,
not merged yet.

Thanks,
Xiaolong
Min, JiaqiX Oct. 29, 2019, 3:12 a.m. UTC | #3 
Hi Xiaolong,

> -----Original Message-----
> From: Ye, Xiaolong
> Sent: Monday, October 28, 2019 4:46 PM
> To: Min, JiaqiX <jiaqix.min@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2] net/ice: fix setting max frame size
> 
> On 10/28, Min JiaqiX wrote:
> >Max frame size is not set to HW, so packets above the MTU do not get
> >dropped by HW. The patch fixed the issue.
> >
> >Fixes: ae2bdd0219cb ("net/ice: support MTU setting")
> 
> We should set max frame size to HW in the beginning when the driver was
> introduced, so the fix commit should be 50370662b727 ("net/ice: support
> device and queue ops")
Modify commit message in patch v3.
> 
> Fix this while merging.
> 
> >Cc: stable@dpdk.org
> >
> >Signed-off-by: Min JiaqiX <jiaqix.min@intel.com>
> >---
> >v2:
> >* Changed commit message.
> >---
> > drivers/net/ice/ice_ethdev.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> >diff --git a/drivers/net/ice/ice_ethdev.c
> >b/drivers/net/ice/ice_ethdev.c index 022b58c01..403305cfb 100644
> >--- a/drivers/net/ice/ice_ethdev.c
> >+++ b/drivers/net/ice/ice_ethdev.c
> >@@ -2506,6 +2506,10 @@ ice_dev_start(struct rte_eth_dev *dev)
> >
> > 	pf->adapter_stopped = false;
> >
> >+	/* Set the max frame size */
> >+	ice_aq_set_mac_cfg(hw,
> >+		pf->dev_data->dev_conf.rxmode.max_rx_pkt_len, NULL);
> >+
> 
> Why set the max frame size to HW in start ops, not in init stage like in i40e?
> and what if max_rx_pkt_len hasn't been set?
> 
max_rx_pkt_len need to be set to HW when set mtu ,  init stage won't be called when open port again.

Appendix:
Steps for using Example/ethtool  to set mtu:
EthApp>stop 0
EthApp>mtu 0 200
EthApp>open 0

> 
> Thanks,
> Xiaolong
> 
> 
> > 	return 0;
> >
> > 	/* stop the started queues if failed to start all queues */
> >--
> >2.17.1
> >
> 
> Reviewed-by: Xiaolong Ye <xiaolong.ye@intel.com>
> 
> Applied to dpdk-next-net-intel. Thanks.
Xiaolong Ye Oct. 29, 2019, 5:07 a.m. UTC | #4 
On 10/29, Min, JiaqiX wrote:
>Hi Xiaolong,
>
>> -----Original Message-----
>> From: Ye, Xiaolong
>> Sent: Monday, October 28, 2019 4:46 PM
>> To: Min, JiaqiX <jiaqix.min@intel.com>
>> Cc: dev@dpdk.org; stable@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH v2] net/ice: fix setting max frame size
>> 
>> On 10/28, Min JiaqiX wrote:
>> >Max frame size is not set to HW, so packets above the MTU do not get
>> >dropped by HW. The patch fixed the issue.
>> >
>> >Fixes: ae2bdd0219cb ("net/ice: support MTU setting")
>> 
>> We should set max frame size to HW in the beginning when the driver was
>> introduced, so the fix commit should be 50370662b727 ("net/ice: support
>> device and queue ops")
>Modify commit message in patch v3.
>> 
>> Fix this while merging.
>> 
>> >Cc: stable@dpdk.org
>> >
>> >Signed-off-by: Min JiaqiX <jiaqix.min@intel.com>
>> >---
>> >v2:
>> >* Changed commit message.
>> >---
>> > drivers/net/ice/ice_ethdev.c | 4 ++++
>> > 1 file changed, 4 insertions(+)
>> >
>> >diff --git a/drivers/net/ice/ice_ethdev.c
>> >b/drivers/net/ice/ice_ethdev.c index 022b58c01..403305cfb 100644
>> >--- a/drivers/net/ice/ice_ethdev.c
>> >+++ b/drivers/net/ice/ice_ethdev.c
>> >@@ -2506,6 +2506,10 @@ ice_dev_start(struct rte_eth_dev *dev)
>> >
>> > 	pf->adapter_stopped = false;
>> >
>> >+	/* Set the max frame size */
>> >+	ice_aq_set_mac_cfg(hw,
>> >+		pf->dev_data->dev_conf.rxmode.max_rx_pkt_len, NULL);
>> >+
>> 
>> Why set the max frame size to HW in start ops, not in init stage like in i40e?
>> and what if max_rx_pkt_len hasn't been set?
>> 
>max_rx_pkt_len need to be set to HW when set mtu ,  init stage won't be called when open port again.

Agree, I think it makes sense to set max frame size to hw in start ops, but what 
if user doesn't set mtu, then the max_rx_pkt_len would be 0, do we need a 
default max frame value in this case? 

And since i40e doesn't update max frame size to HW in start ops, do you think
it needs a fix?

Thanks,
Xiaolong


>
>Appendix:
>Steps for using Example/ethtool  to set mtu:
>EthApp>stop 0
>EthApp>mtu 0 200
>EthApp>open 0
>
>> 
>> Thanks,
>> Xiaolong
>> 
>> 
>> > 	return 0;
>> >
>> > 	/* stop the started queues if failed to start all queues */
>> >--
>> >2.17.1
>> >
>> 
>> Reviewed-by: Xiaolong Ye <xiaolong.ye@intel.com>
>> 
>> Applied to dpdk-next-net-intel. Thanks.
Min, JiaqiX Oct. 30, 2019, 6:34 a.m. UTC | #5 
Hi, Xiaolong

> -----Original Message-----
> From: Ye, Xiaolong
> Sent: Tuesday, October 29, 2019 1:07 PM
> To: Min, JiaqiX <jiaqix.min@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2] net/ice: fix setting max frame size
> 
> On 10/29, Min, JiaqiX wrote:
> >Hi Xiaolong,
> >
> >> -----Original Message-----
> >> From: Ye, Xiaolong
> >> Sent: Monday, October 28, 2019 4:46 PM
> >> To: Min, JiaqiX <jiaqix.min@intel.com>
> >> Cc: dev@dpdk.org; stable@dpdk.org
> >> Subject: Re: [dpdk-dev] [PATCH v2] net/ice: fix setting max frame
> >> size
> >>
> >> On 10/28, Min JiaqiX wrote:
> >> >Max frame size is not set to HW, so packets above the MTU do not get
> >> >dropped by HW. The patch fixed the issue.
> >> >
> >> >Fixes: ae2bdd0219cb ("net/ice: support MTU setting")
> >>
> >> We should set max frame size to HW in the beginning when the driver
> >> was introduced, so the fix commit should be 50370662b727 ("net/ice:
> >> support device and queue ops")
> >Modify commit message in patch v3.
> >>
> >> Fix this while merging.
> >>
> >> >Cc: stable@dpdk.org
> >> >
> >> >Signed-off-by: Min JiaqiX <jiaqix.min@intel.com>
> >> >---
> >> >v2:
> >> >* Changed commit message.
> >> >---
> >> > drivers/net/ice/ice_ethdev.c | 4 ++++
> >> > 1 file changed, 4 insertions(+)
> >> >
> >> >diff --git a/drivers/net/ice/ice_ethdev.c
> >> >b/drivers/net/ice/ice_ethdev.c index 022b58c01..403305cfb 100644
> >> >--- a/drivers/net/ice/ice_ethdev.c
> >> >+++ b/drivers/net/ice/ice_ethdev.c
> >> >@@ -2506,6 +2506,10 @@ ice_dev_start(struct rte_eth_dev *dev)
> >> >
> >> > 	pf->adapter_stopped = false;
> >> >
> >> >+	/* Set the max frame size */
> >> >+	ice_aq_set_mac_cfg(hw,
> >> >+		pf->dev_data->dev_conf.rxmode.max_rx_pkt_len, NULL);
> >> >+
> >>
> >> Why set the max frame size to HW in start ops, not in init stage like in i40e?
> >> and what if max_rx_pkt_len hasn't been set?
> >>
> >max_rx_pkt_len need to be set to HW when set mtu ,  init stage won't be
> called when open port again.
> 
> Agree, I think it makes sense to set max frame size to hw in start ops, but
> what if user doesn't set mtu, then the max_rx_pkt_len would be 0, do we
> need a default max frame value in this case?
Agree, we should  set a default max frame size value to HW.

> 
> And since i40e doesn't update max frame size to HW in start ops, do you
> think it needs a fix?
It can't fix the issue to set max frame size to HW in start ops  in i40e, I don't know the cause.
It is ok to set max frame size to HW in link_update ops  in i40e, 
however when user opens port,  link_update ops will be called three times, 
so I don't think it makes sense to set max frame size to hw in link_update ops too.
Where to set the maximum frame size to HW in i40e is a problem.

> 
> Thanks,
> Xiaolong
> 
> 
> >
> >Appendix:
> >Steps for using Example/ethtool  to set mtu:
> >EthApp>stop 0
> >EthApp>mtu 0 200
> >EthApp>open 0
> >
> >>
> >> Thanks,
> >> Xiaolong
> >>
> >>
> >> > 	return 0;
> >> >
> >> > 	/* stop the started queues if failed to start all queues */
> >> >--
> >> >2.17.1
> >> >
> >>
> >> Reviewed-by: Xiaolong Ye <xiaolong.ye@intel.com>
> >>
> >> Applied to dpdk-next-net-intel. Thanks.
diff mbox series

Patch

diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
index 022b58c01..403305cfb 100644
--- a/drivers/net/ice/ice_ethdev.c
+++ b/drivers/net/ice/ice_ethdev.c
@@ -2506,6 +2506,10 @@  ice_dev_start(struct rte_eth_dev *dev)
 
 	pf->adapter_stopped = false;
 
+	/* Set the max frame size */
+	ice_aq_set_mac_cfg(hw,
+		pf->dev_data->dev_conf.rxmode.max_rx_pkt_len, NULL);
+
 	return 0;
 
 	/* stop the started queues if failed to start all queues */