Re: [ovs-dev] [PATCH] netdev-dpdk: defer MTU set after interface start

Re: [ovs-dev] [PATCH] netdev-dpdk: defer MTU set after interface start

[Jan Scheurich]

> > The issue only arises with the qede PMD and 67fe6d635193
> > ("netdev-dpdk: use rte_eth_dev_set_mtu.")
>
> I had some more time to look at this today but this patch will break OVS DPDK for existing supported DPDK ports during testing.
>
> I tested with an XL710 and the MTU will fail to apply, the device must be stopped before configuration changes can be applied such as
> MTU. See log message below
>
> 2017-11-28T17:13:50Z|00086|dpdk|ERR|i40e_dev_mtu_set(): port 0 must be stopped before configuration
> 2017-11-28T17:13:50Z|00087|netdev_dpdk|ERR|Interface dpdk0 MTU (1500) setup error: Device or resource busy
>
> Did you come across it in your testing? I guess you didn’t see this for QEDE pmd. In my case the DPDK devices will simply fail to add to the
> bridge.
>
> As is, the patch would not be acceptable as its breaking existing functionality. It would have to be reworked to configure for device that
> cannot reconfigure when busy.
>
> Thanks
> Ian

I fully support the original decision to switch to using rte_dev_set_mtu() in OVS. The prior approach setting max_rx_pkt_len in rte_eth_dev_configure() was non-portable as that field has no well-defined semantics and its relation to the MTU size is different for almost every PMD.

I had a look at the qede PMD implementation of rte_dev_set_mtu() in DPDK 17.05 and 17.11. It assumes that the device must be started because qede_set_mtu() unconditionally stops the device before and restarts it after changing the MTU value. Given that other PMDs like i40e don’t accept it after start, there is no possible way OVS can use rte_dev_set_mtu() that will work with all drivers.

I think it is a weakness of the rte_ethdev API that it does not specify clearly when API functions like rte_dev_set_mtu() may be called. From the documentation of error -EBUSY one can guess that the intention was to optionally support it when the device is started. Implicitly one could conclude that it MUST be supported when the device stopped. That is logical and also what most PMDs do.

I would say the qede PMD should be fixed. It should accept the rte_dev_set_mtu() at any time between rte_eth_dev_configure() and rte_eth_dev_start() (and optionally also after start).

BR, Jan

Re: [PATCH] netdev-dpdk: defer MTU set after interface start

[Stokes, Ian]

> > The issue only arises with the qede PMD and 67fe6d635193
> > ("netdev-dpdk: use rte_eth_dev_set_mtu.")
>
> I had some more time to look at this today but this patch will break OVS DPDK for existing supported DPDK ports during testing.
>
> I tested with an XL710 and the MTU will fail to apply, the device must be stopped before configuration changes can be applied such as
> MTU. See log message below
>
> 2017-11-28T17:13:50Z|00086|dpdk|ERR|i40e_dev_mtu_set(): port 0 must be stopped before configuration
> 2017-11-28T17:13:50Z|00087|netdev_dpdk|ERR|Interface dpdk0 MTU (1500) setup error: Device or resource busy
>
> Did you come across it in your testing? I guess you didn’t see this for QEDE pmd. In my case the DPDK devices will simply fail to add to the
> bridge.
>
> As is, the patch would not be acceptable as its breaking existing functionality. It would have to be reworked to configure for device that
> cannot reconfigure when busy.
>
> Thanks
> Ian

I fully support the original decision to switch to using rte_dev_set_mtu() in OVS. The prior approach setting max_rx_pkt_len in rte_eth_dev_configure() was non-portable as that field has no well-defined semantics and its relation to the MTU size is different for almost every PMD.

I had a look at the qede PMD implementation of rte_dev_set_mtu() in DPDK 17.05 and 17.11. It assumes that the device must be started because qede_set_mtu() unconditionally stops the device before and restarts it after changing the MTU value. Given that other PMDs like i40e don’t accept it after start, there is no possible way OVS can use rte_dev_set_mtu() that will work with all drivers.

I think it is a weakness of the rte_ethdev API that it does not specify clearly when API functions like rte_dev_set_mtu() may be called. From the documentation of error -EBUSY one can guess that the intention was to optionally support it when the device is started. Implicitly one could conclude that it MUST be supported when the device stopped. That is logical and also what most PMDs do.

I would say the qede PMD should be fixed. It should accept the rte_dev_set_mtu() at any time between rte_eth_dev_configure() and rte_eth_dev_start() (and optionally also after start).

BR, Jan

[IS] Thanks for doing this follow up Jan, it’s really valuable in my opinion.

It’s definitely something that should be raised to the DPDK community as conceivably if there is no set process to follow we could see varying behavior from PMD device to device. I’m happy to take this and raise it to the DPDK community.

Thanks
Ian

_______________________________________________
dev mailing list
dev@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] netdev-dpdk: defer MTU set after interface start

[Aaron Conole]

"Stokes, Ian" <ian.stokes@intel.com> writes:

>> > The issue only arises with the qede PMD and 67fe6d635193
>> > ("netdev-dpdk: use rte_eth_dev_set_mtu.")
>>
>> I had some more time to look at this today but this patch will break
>> OVS DPDK for existing supported DPDK ports during testing.
>>
>> I tested with an XL710 and the MTU will fail to apply, the device
>> must be stopped before configuration changes can be applied such as
>> MTU. See log message below
>>
>> 2017-11-28T17:13:50Z|00086|dpdk|ERR|i40e_dev_mtu_set(): port 0 must
>> be stopped before configuration
>> 2017-11-28T17:13:50Z|00087|netdev_dpdk|ERR|Interface dpdk0 MTU
>> (1500) setup error: Device or resource busy
>>
>> Did you come across it in your testing? I guess you didn’t see this
>> for QEDE pmd. In my case the DPDK devices will simply fail to add to
>> the
>> bridge.
>>
>> As is, the patch would not be acceptable as its breaking existing
>> functionality. It would have to be reworked to configure for device
>> that
>> cannot reconfigure when busy.
>>
>> Thanks
>> Ian
>
> I fully support the original decision to switch to using
> rte_dev_set_mtu() in OVS. The prior approach setting max_rx_pkt_len in
> rte_eth_dev_configure() was non-portable as that field has no
> well-defined semantics and its relation to the MTU size is different
> for almost every PMD.
>
> I had a look at the qede PMD implementation of rte_dev_set_mtu() in
> DPDK 17.05 and 17.11. It assumes that the device must be started
> because qede_set_mtu() unconditionally stops the device before and
> restarts it after changing the MTU value. Given that other PMDs like
> i40e don’t accept it after start, there is no possible way OVS can use
> rte_dev_set_mtu() that will work with all drivers.
>
> I think it is a weakness of the rte_ethdev API that it does not
> specify clearly when API functions like rte_dev_set_mtu() may be
> called. From the documentation of error -EBUSY one can guess that the
> intention was to optionally support it when the device is
> started. Implicitly one could conclude that it MUST be supported when
> the device stopped. That is logical and also what most PMDs do.
>
> I would say the qede PMD should be fixed. It should accept the
> rte_dev_set_mtu() at any time between rte_eth_dev_configure() and
> rte_eth_dev_start() (and optionally also after start).
>
> BR, Jan
>
> [IS] Thanks for doing this follow up Jan, it’s really valuable in my opinion.
>
> It’s definitely something that should be raised to the DPDK community
> as conceivably if there is no set process to follow we could see
> varying behavior from PMD device to device. I’m happy to take this and
> raise it to the DPDK community.

Agreed that the understanding of when/how this setter can be called is
confusing.  I contacted Harish Patil (who works on that driver for the
DPDK side) about this, and think that there's a followup to this thread
coming.

> Thanks
> Ian
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev