[dpdk-dev,v4,1/3] lib/librte_ether: add support for port reset
Checks
Commit Message
Add support for port reset in rte layer.This reset
feature can not only used in vf port reset in later code
develop, but alsopf port.But in this patch set, we only
limit the discussion scope to vf reset.
This patch Add an API to restart the device.
It's for VF device in this scenario, kernel PF + DPDK VF.
When the PF port down->up, APP should call this API to
restart VF port. Most likely, APP should call it in its
management thread and guarantee the thread safe. It means
APP should stop the rx/tx and the device, then restart
the device, then recover the device and rx/tx.
Also, it's APP's responsibilty to release the occupied
memory.
Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
---
lib/librte_ether/rte_ethdev.c | 17 +++++++++++++++++
lib/librte_ether/rte_ethdev.h | 25 +++++++++++++++++++++++++
lib/librte_ether/rte_ether_version.map | 6 ++++++
3 files changed, 48 insertions(+)
Comments
Hi,
Please help reviewers, use --in-reply-to to keep patches threaded.
2017-03-30 17:34, Wei Zhao:
> Add support for port reset in rte layer.This reset
> feature can not only used in vf port reset in later code
> develop, but alsopf port.But in this patch set, we only
> limit the discussion scope to vf reset.
> This patch Add an API to restart the device.
> It's for VF device in this scenario, kernel PF + DPDK VF.
> When the PF port down->up, APP should call this API to
> restart VF port. Most likely, APP should call it in its
> management thread and guarantee the thread safe. It means
> APP should stop the rx/tx and the device, then restart
> the device, then recover the device and rx/tx.
> Also, it's APP's responsibilty to release the occupied
> memory.
Which memory should be released?
[...]
/**
> + * Reset an ethernet device when it's not working. One scenario is, after PF
> + * port is down and up, the related VF port should be reset.
> + * The API will stop the port, clear the rx/tx queues, re-setup the rx/tx
> + * queues, restart the port.
s/The API/This function/
Please explain exactly the responsibility of this function,
and how it is different from calling stop/configure/start.
> + * Before calling this API, APP should stop the rx/tx. When tx is being stopped,
> + * APP can drop the packets and release the buffer instead of sending them.
> + *
> + * @param port_id
> + * The port identifier of the Ethernet device.
> + *
> + * @return
> + * - (0) if successful.
> + * - (-ENODEV) if port identifier is invalid.
> + * - (-ENOTSUP) if hardware doesn't support this function.
> + */
> +int
> +rte_eth_dev_reset(uint8_t port_id);
Please John, could you help with the API documentation here?
Hi, Thomas
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Friday, March 31, 2017 3:55 AM
> To: Zhao1, Wei <wei.zhao1@intel.com>; Mcnamara, John
> <john.mcnamara@intel.com>
> Cc: dev@dpdk.org; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v4 1/3] lib/librte_ether: add support for port
> reset
>
> Hi,
>
> Please help reviewers, use --in-reply-to to keep patches threaded.
>
Ok， I will use that use --in-reply-to in later when commit patch.
> 2017-03-30 17:34, Wei Zhao:
> > Add support for port reset in rte layer.This reset feature can not
> > only used in vf port reset in later code develop, but alsopf port.But
> > in this patch set, we only limit the discussion scope to vf reset.
> > This patch Add an API to restart the device.
> > It's for VF device in this scenario, kernel PF + DPDK VF.
> > When the PF port down->up, APP should call this API to restart VF
> > port. Most likely, APP should call it in its management thread and
> > guarantee the thread safe. It means APP should stop the rx/tx and the
> > device, then restart the device, then recover the device and rx/tx.
> > Also, it's APP's responsibilty to release the occupied memory.
>
> Which memory should be released?
That is a redundancy now, because the older version need to allocation some memory for this feature,
But it do not need now. So I will delete it in next version.
>
> [...]
> /**
> > + * Reset an ethernet device when it's not working. One scenario is,
> > + after PF
> > + * port is down and up, the related VF port should be reset.
> > + * The API will stop the port, clear the rx/tx queues, re-setup the
> > + rx/tx
> > + * queues, restart the port.
>
> s/The API/This function/
>
> Please explain exactly the responsibility of this function, and how it is
> different from calling stop/configure/start.
In this reset feature, reset function can do the calling stop/configure/start process, but also
It can also do some restore work for the port, for example, it can restore the added parameters
of vlan, mac_addrs, promisc_unicast_enabled falg and promisc_multicast_enabled flag.
Maybe , I should add this explanation in the patch comments or function comments?
>
> > + * Before calling this API, APP should stop the rx/tx. When tx is
> > +being stopped,
> > + * APP can drop the packets and release the buffer instead of sending
> them.
> > + *
> > + * @param port_id
> > + * The port identifier of the Ethernet device.
> > + *
> > + * @return
> > + * - (0) if successful.
> > + * - (-ENODEV) if port identifier is invalid.
> > + * - (-ENOTSUP) if hardware doesn't support this function.
> > + */
> > +int
> > +rte_eth_dev_reset(uint8_t port_id);
>
> Please John, could you help with the API documentation here?
Thank you for John, please contact me if any puzzle.
2017-04-06 02:57, Zhao1, Wei:
> > /**
> > > + * Reset an ethernet device when it's not working. One scenario is,
> > > + after PF
> > > + * port is down and up, the related VF port should be reset.
> > > + * The API will stop the port, clear the rx/tx queues, re-setup the
> > > + rx/tx
> > > + * queues, restart the port.
> >
> > s/The API/This function/
> >
> > Please explain exactly the responsibility of this function, and how it is
> > different from calling stop/configure/start.
>
> In this reset feature, reset function can do the calling stop/configure/start process, but also
> It can also do some restore work for the port, for example, it can restore the added parameters
> of vlan, mac_addrs, promisc_unicast_enabled falg and promisc_multicast_enabled flag.
> Maybe , I should add this explanation in the patch comments or function comments?
Yes it must be explain in the doxygen part of the function.
Hi, Thomas
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Thursday, April 6, 2017 3:11 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>
> Cc: Mcnamara, John <john.mcnamara@intel.com>; dev@dpdk.org; Lu,
> Wenzhuo <wenzhuo.lu@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v4 1/3] lib/librte_ether: add support for port
> reset
>
> 2017-04-06 02:57, Zhao1, Wei:
> > > /**
> > > > + * Reset an ethernet device when it's not working. One scenario
> > > > + is, after PF
> > > > + * port is down and up, the related VF port should be reset.
> > > > + * The API will stop the port, clear the rx/tx queues, re-setup
> > > > + the rx/tx
> > > > + * queues, restart the port.
> > >
> > > s/The API/This function/
> > >
> > > Please explain exactly the responsibility of this function, and how
> > > it is different from calling stop/configure/start.
> >
> > In this reset feature, reset function can do the calling
> > stop/configure/start process, but also It can also do some restore
> > work for the port, for example, it can restore the added parameters of
> vlan, mac_addrs, promisc_unicast_enabled falg and
> promisc_multicast_enabled flag.
> > Maybe , I should add this explanation in the patch comments or function
> comments?
>
> Yes it must be explain in the doxygen part of the function.
Yes, I have add that explanation in v5 which has been commit to dpdk.org.
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Zhao1, Wei
> Sent: Thursday, April 6, 2017 9:53 AM
> To: Thomas Monjalon <thomas.monjalon@6wind.com>
> Cc: Mcnamara, John <john.mcnamara@intel.com>; dev@dpdk.org; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v4 1/3] lib/librte_ether: add support for port reset
>
> Hi, Thomas
>
> > -----Original Message-----
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > Sent: Thursday, April 6, 2017 3:11 PM
> > To: Zhao1, Wei <wei.zhao1@intel.com>
> > Cc: Mcnamara, John <john.mcnamara@intel.com>; dev@dpdk.org; Lu,
> > Wenzhuo <wenzhuo.lu@intel.com>
> > Subject: Re: [dpdk-dev] [PATCH v4 1/3] lib/librte_ether: add support for port
> > reset
> >
> > 2017-04-06 02:57, Zhao1, Wei:
> > > > /**
> > > > > + * Reset an ethernet device when it's not working. One scenario
> > > > > + is, after PF
> > > > > + * port is down and up, the related VF port should be reset.
> > > > > + * The API will stop the port, clear the rx/tx queues, re-setup
> > > > > + the rx/tx
> > > > > + * queues, restart the port.
> > > >
> > > > s/The API/This function/
> > > >
> > > > Please explain exactly the responsibility of this function, and how
> > > > it is different from calling stop/configure/start.
> > >
> > > In this reset feature, reset function can do the calling
> > > stop/configure/start process, but also It can also do some restore
> > > work for the port, for example, it can restore the added parameters of
> > vlan, mac_addrs, promisc_unicast_enabled falg and
> > promisc_multicast_enabled flag.
Ok, but why start/stop can't do these things?
Konstantin
> > > Maybe , I should add this explanation in the patch comments or function
> > comments?
> >
> > Yes it must be explain in the doxygen part of the function.
>
> Yes, I have add that explanation in v5 which has been commit to dpdk.org.
2017-04-06 09:02, Ananyev, Konstantin:
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Zhao1, Wei
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > 2017-04-06 02:57, Zhao1, Wei:
> > > > > /**
> > > > > > + * Reset an ethernet device when it's not working. One scenario
> > > > > > + is, after PF
> > > > > > + * port is down and up, the related VF port should be reset.
> > > > > > + * The API will stop the port, clear the rx/tx queues, re-setup
> > > > > > + the rx/tx
> > > > > > + * queues, restart the port.
> > > > >
> > > > > s/The API/This function/
> > > > >
> > > > > Please explain exactly the responsibility of this function, and how
> > > > > it is different from calling stop/configure/start.
> > > >
> > > > In this reset feature, reset function can do the calling
> > > > stop/configure/start process, but also It can also do some restore
> > > > work for the port, for example, it can restore the added parameters of
> > > vlan, mac_addrs, promisc_unicast_enabled falg and
> > > promisc_multicast_enabled flag.
>
> Ok, but why start/stop can't do these things?
Please could you try to answer this question?
We cannot accept v7 if there are some doubts remaining.
Hi, Konstantin
> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Thursday, April 6, 2017 5:03 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>; Thomas Monjalon
> <thomas.monjalon@6wind.com>
> Cc: Mcnamara, John <john.mcnamara@intel.com>; dev@dpdk.org; Lu,
> Wenzhuo <wenzhuo.lu@intel.com>
> Subject: RE: [dpdk-dev] [PATCH v4 1/3] lib/librte_ether: add support for port
> reset
>
>
>
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Zhao1, Wei
> > Sent: Thursday, April 6, 2017 9:53 AM
> > To: Thomas Monjalon <thomas.monjalon@6wind.com>
> > Cc: Mcnamara, John <john.mcnamara@intel.com>; dev@dpdk.org; Lu,
> > Wenzhuo <wenzhuo.lu@intel.com>
> > Subject: Re: [dpdk-dev] [PATCH v4 1/3] lib/librte_ether: add support
> > for port reset
> >
> > Hi, Thomas
> >
> > > -----Original Message-----
> > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > Sent: Thursday, April 6, 2017 3:11 PM
> > > To: Zhao1, Wei <wei.zhao1@intel.com>
> > > Cc: Mcnamara, John <john.mcnamara@intel.com>; dev@dpdk.org; Lu,
> > > Wenzhuo <wenzhuo.lu@intel.com>
> > > Subject: Re: [dpdk-dev] [PATCH v4 1/3] lib/librte_ether: add support
> > > for port reset
> > >
> > > 2017-04-06 02:57, Zhao1, Wei:
> > > > > /**
> > > > > > + * Reset an ethernet device when it's not working. One
> > > > > > + scenario is, after PF
> > > > > > + * port is down and up, the related VF port should be reset.
> > > > > > + * The API will stop the port, clear the rx/tx queues,
> > > > > > + re-setup the rx/tx
> > > > > > + * queues, restart the port.
> > > > >
> > > > > s/The API/This function/
> > > > >
> > > > > Please explain exactly the responsibility of this function, and
> > > > > how it is different from calling stop/configure/start.
> > > >
> > > > In this reset feature, reset function can do the calling
> > > > stop/configure/start process, but also It can also do some restore
> > > > work for the port, for example, it can restore the added
> > > > parameters of
> > > vlan, mac_addrs, promisc_unicast_enabled falg and
> > > promisc_multicast_enabled flag.
>
> Ok, but why start/stop can't do these things?
> Konstantin
This is because in i40e PMD code, start and stop process do not have the process of store and restore
the added key parameters. Not only i40e but also other PMD code. So, in the function pointed to by dev_reset,
we add specific function do store and restore of some of the important parameters listed above.
>
> > > > Maybe , I should add this explanation in the patch comments or
> > > > function
> > > comments?
> > >
> > > Yes it must be explain in the doxygen part of the function.
> >
> > Yes, I have add that explanation in v5 which has been commit to dpdk.org.
2017-04-13 08:55, Zhao1, Wei:
> From: Ananyev, Konstantin
> > From: Zhao1, Wei
> > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > 2017-04-06 02:57, Zhao1, Wei:
> > > > > > /**
> > > > > > > + * Reset an ethernet device when it's not working. One
> > > > > > > + scenario is, after PF
> > > > > > > + * port is down and up, the related VF port should be reset.
> > > > > > > + * The API will stop the port, clear the rx/tx queues,
> > > > > > > + re-setup the rx/tx
> > > > > > > + * queues, restart the port.
> > > > > >
> > > > > > s/The API/This function/
> > > > > >
> > > > > > Please explain exactly the responsibility of this function, and
> > > > > > how it is different from calling stop/configure/start.
> > > > >
> > > > > In this reset feature, reset function can do the calling
> > > > > stop/configure/start process, but also It can also do some restore
> > > > > work for the port, for example, it can restore the added
> > > > > parameters of
> > > > vlan, mac_addrs, promisc_unicast_enabled falg and
> > > > promisc_multicast_enabled flag.
> >
> > Ok, but why start/stop can't do these things?
> > Konstantin
>
> This is because in i40e PMD code, start and stop process do not have the process of store and restore
> the added key parameters. Not only i40e but also other PMD code. So, in the function pointed to by dev_reset,
> we add specific function do store and restore of some of the important parameters listed above.
Why store and restore cannot be implemented in start/stop functions?
Hi, Thomas Monjalon
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Thursday, April 13, 2017 6:07 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Mcnamara, John
> <john.mcnamara@intel.com>; dev@dpdk.org; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v4 1/3] lib/librte_ether: add support for port
> reset
>
> 2017-04-13 08:55, Zhao1, Wei:
> > From: Ananyev, Konstantin
> > > From: Zhao1, Wei
> > > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > > 2017-04-06 02:57, Zhao1, Wei:
> > > > > > > /**
> > > > > > > > + * Reset an ethernet device when it's not working. One
> > > > > > > > + scenario is, after PF
> > > > > > > > + * port is down and up, the related VF port should be reset.
> > > > > > > > + * The API will stop the port, clear the rx/tx queues,
> > > > > > > > + re-setup the rx/tx
> > > > > > > > + * queues, restart the port.
> > > > > > >
> > > > > > > s/The API/This function/
> > > > > > >
> > > > > > > Please explain exactly the responsibility of this function,
> > > > > > > and how it is different from calling stop/configure/start.
> > > > > >
> > > > > > In this reset feature, reset function can do the calling
> > > > > > stop/configure/start process, but also It can also do some
> > > > > > restore work for the port, for example, it can restore the
> > > > > > added parameters of
> > > > > vlan, mac_addrs, promisc_unicast_enabled falg and
> > > > > promisc_multicast_enabled flag.
> > >
> > > Ok, but why start/stop can't do these things?
> > > Konstantin
> >
> > This is because in i40e PMD code, start and stop process do not have
> > the process of store and restore the added key parameters. Not only
> > i40e but also other PMD code. So, in the function pointed to by dev_reset,
> we add specific function do store and restore of some of the important
> parameters listed above.
>
> Why store and restore cannot be implemented in start/stop functions?
Because reset and start/stop are used for two purposes, for example:
Some user maybe just start/stop the port and he do not care what key parameters
has been configuration last time, and even worse when he want to clear all the configuration last time ,
if we add specific function do store and restore in that two function, it is useless for them,
and may cause a result that user do not expect.
2017-04-14 01:29, Zhao1, Wei:
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > 2017-04-13 08:55, Zhao1, Wei:
> > > From: Ananyev, Konstantin
> > > > From: Zhao1, Wei
> > > > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > > > 2017-04-06 02:57, Zhao1, Wei:
> > > > > > > > /**
> > > > > > > > > + * Reset an ethernet device when it's not working. One
> > > > > > > > > + scenario is, after PF
> > > > > > > > > + * port is down and up, the related VF port should be reset.
> > > > > > > > > + * The API will stop the port, clear the rx/tx queues,
> > > > > > > > > + re-setup the rx/tx
> > > > > > > > > + * queues, restart the port.
> > > > > > > >
> > > > > > > > s/The API/This function/
> > > > > > > >
> > > > > > > > Please explain exactly the responsibility of this function,
> > > > > > > > and how it is different from calling stop/configure/start.
> > > > > > >
> > > > > > > In this reset feature, reset function can do the calling
> > > > > > > stop/configure/start process, but also It can also do some
> > > > > > > restore work for the port, for example, it can restore the
> > > > > > > added parameters of
> > > > > > vlan, mac_addrs, promisc_unicast_enabled falg and
> > > > > > promisc_multicast_enabled flag.
> > > >
> > > > Ok, but why start/stop can't do these things?
> > > > Konstantin
> > >
> > > This is because in i40e PMD code, start and stop process do not have
> > > the process of store and restore the added key parameters. Not only
> > > i40e but also other PMD code. So, in the function pointed to by dev_reset,
> > we add specific function do store and restore of some of the important
> > parameters listed above.
> >
> > Why store and restore cannot be implemented in start/stop functions?
>
> Because reset and start/stop are used for two purposes, for example:
> Some user maybe just start/stop the port and he do not care what key parameters
> has been configuration last time, and even worse when he want to clear all the configuration last time ,
> if we add specific function do store and restore in that two function, it is useless for them,
> and may cause a result that user do not expect.
Is it said somewhere in the doc that the configuration is lost when
stopping a device?
Can we say which configuration parameter is kept and which one is lost?
rte_eth_dev_config_restore() is called in rte_eth_dev_start().
Hi, Thomas
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Friday, April 14, 2017 2:31 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Mcnamara, John
> <john.mcnamara@intel.com>; dev@dpdk.org; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v4 1/3] lib/librte_ether: add support for port
> reset
>
> 2017-04-14 01:29, Zhao1, Wei:
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > 2017-04-13 08:55, Zhao1, Wei:
> > > > From: Ananyev, Konstantin
> > > > > From: Zhao1, Wei
> > > > > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > > > > 2017-04-06 02:57, Zhao1, Wei:
> > > > > > > > > /**
> > > > > > > > > > + * Reset an ethernet device when it's not working.
> > > > > > > > > > + One scenario is, after PF
> > > > > > > > > > + * port is down and up, the related VF port should be reset.
> > > > > > > > > > + * The API will stop the port, clear the rx/tx
> > > > > > > > > > + queues, re-setup the rx/tx
> > > > > > > > > > + * queues, restart the port.
> > > > > > > > >
> > > > > > > > > s/The API/This function/
> > > > > > > > >
> > > > > > > > > Please explain exactly the responsibility of this
> > > > > > > > > function, and how it is different from calling
> stop/configure/start.
> > > > > > > >
> > > > > > > > In this reset feature, reset function can do the calling
> > > > > > > > stop/configure/start process, but also It can also do some
> > > > > > > > restore work for the port, for example, it can restore the
> > > > > > > > added parameters of
> > > > > > > vlan, mac_addrs, promisc_unicast_enabled falg and
> > > > > > > promisc_multicast_enabled flag.
> > > > >
> > > > > Ok, but why start/stop can't do these things?
> > > > > Konstantin
> > > >
> > > > This is because in i40e PMD code, start and stop process do not
> > > > have the process of store and restore the added key parameters.
> > > > Not only i40e but also other PMD code. So, in the function
> > > > pointed to by dev_reset,
> > > we add specific function do store and restore of some of the
> > > important parameters listed above.
> > >
> > > Why store and restore cannot be implemented in start/stop functions?
> >
> > Because reset and start/stop are used for two purposes, for example:
> > Some user maybe just start/stop the port and he do not care what key
> > parameters has been configuration last time, and even worse when he
> > want to clear all the configuration last time , if we add specific
> > function do store and restore in that two function, it is useless for them,
> and may cause a result that user do not expect.
>
> Is it said somewhere in the doc that the configuration is lost when stopping a
> device?
> Can we say which configuration parameter is kept and which one is lost?
>
> rte_eth_dev_config_restore() is called in rte_eth_dev_start().
Port reset process not only involve the rte_eth_dev_start() and rte_eth_dev_stop().
It also involve eth_dev_uninit() and eth_dev_init() process，
in which PMD device uninit and init. In this case, for example, data->mac_addrs buffer is freed so it need to add store and restore function.
BUT, if you only call stop/configure/start process, that is not strictly what named "device reset".
Hi, Thomas
All questions about this patch set has been answered, is it clear or not?And is there anything that I should do for it?
Or it is OK for merge into 17.02-RC2?
Thank you.
> -----Original Message-----
> From: Zhao1, Wei
> Sent: Friday, April 14, 2017 4:03 PM
> To: Thomas Monjalon <thomas.monjalon@6wind.com>
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Mcnamara, John
> <john.mcnamara@intel.com>; dev@dpdk.org; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>
> Subject: RE: [dpdk-dev] [PATCH v4 1/3] lib/librte_ether: add support for port
> reset
>
> Hi, Thomas
>
> > -----Original Message-----
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > Sent: Friday, April 14, 2017 2:31 PM
> > To: Zhao1, Wei <wei.zhao1@intel.com>
> > Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Mcnamara,
> John
> > <john.mcnamara@intel.com>; dev@dpdk.org; Lu, Wenzhuo
> > <wenzhuo.lu@intel.com>
> > Subject: Re: [dpdk-dev] [PATCH v4 1/3] lib/librte_ether: add support
> > for port reset
> >
> > 2017-04-14 01:29, Zhao1, Wei:
> > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > 2017-04-13 08:55, Zhao1, Wei:
> > > > > From: Ananyev, Konstantin
> > > > > > From: Zhao1, Wei
> > > > > > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > > > > > 2017-04-06 02:57, Zhao1, Wei:
> > > > > > > > > > /**
> > > > > > > > > > > + * Reset an ethernet device when it's not working.
> > > > > > > > > > > + One scenario is, after PF
> > > > > > > > > > > + * port is down and up, the related VF port should be
> reset.
> > > > > > > > > > > + * The API will stop the port, clear the rx/tx
> > > > > > > > > > > + queues, re-setup the rx/tx
> > > > > > > > > > > + * queues, restart the port.
> > > > > > > > > >
> > > > > > > > > > s/The API/This function/
> > > > > > > > > >
> > > > > > > > > > Please explain exactly the responsibility of this
> > > > > > > > > > function, and how it is different from calling
> > stop/configure/start.
> > > > > > > > >
> > > > > > > > > In this reset feature, reset function can do the calling
> > > > > > > > > stop/configure/start process, but also It can also do
> > > > > > > > > some restore work for the port, for example, it can
> > > > > > > > > restore the added parameters of
> > > > > > > > vlan, mac_addrs, promisc_unicast_enabled falg and
> > > > > > > > promisc_multicast_enabled flag.
> > > > > >
> > > > > > Ok, but why start/stop can't do these things?
> > > > > > Konstantin
> > > > >
> > > > > This is because in i40e PMD code, start and stop process do not
> > > > > have the process of store and restore the added key parameters.
> > > > > Not only i40e but also other PMD code. So, in the function
> > > > > pointed to by dev_reset,
> > > > we add specific function do store and restore of some of the
> > > > important parameters listed above.
> > > >
> > > > Why store and restore cannot be implemented in start/stop functions?
> > >
> > > Because reset and start/stop are used for two purposes, for example:
> > > Some user maybe just start/stop the port and he do not care what
> > > key parameters has been configuration last time, and even worse when
> > > he want to clear all the configuration last time , if we add
> > > specific function do store and restore in that two function, it is
> > > useless for them,
> > and may cause a result that user do not expect.
> >
> > Is it said somewhere in the doc that the configuration is lost when
> > stopping a device?
> > Can we say which configuration parameter is kept and which one is lost?
> >
> > rte_eth_dev_config_restore() is called in rte_eth_dev_start().
>
> Port reset process not only involve the rte_eth_dev_start() and
> rte_eth_dev_stop().
> It also involve eth_dev_uninit() and eth_dev_init() process，
> in which PMD device uninit and init. In this case, for example, data-
> >mac_addrs buffer is freed so it need to add store and restore function.
> BUT, if you only call stop/configure/start process, that is not strictly what
> named "device reset".
Add Thomas new mail address for this mail.
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Zhao1, Wei
> Sent: Monday, April 17, 2017 10:09 AM
> To: 'Thomas Monjalon' <thomas.monjalon@6wind.com>
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Mcnamara, John
> <john.mcnamara@intel.com>; 'dev@dpdk.org' <dev@dpdk.org>; Lu,
> Wenzhuo <wenzhuo.lu@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v4 1/3] lib/librte_ether: add support for port
> reset
>
> Hi, Thomas
>
> All questions about this patch set has been answered, is it clear or not?And
> is there anything that I should do for it?
> Or it is OK for merge into 17.02-RC2?
>
> Thank you.
>
> > -----Original Message-----
> > From: Zhao1, Wei
> > Sent: Friday, April 14, 2017 4:03 PM
> > To: Thomas Monjalon <thomas.monjalon@6wind.com>
> > Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Mcnamara,
> John
> > <john.mcnamara@intel.com>; dev@dpdk.org; Lu, Wenzhuo
> > <wenzhuo.lu@intel.com>
> > Subject: RE: [dpdk-dev] [PATCH v4 1/3] lib/librte_ether: add support
> > for port reset
> >
> > Hi, Thomas
> >
> > > -----Original Message-----
> > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > Sent: Friday, April 14, 2017 2:31 PM
> > > To: Zhao1, Wei <wei.zhao1@intel.com>
> > > Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Mcnamara,
> > John
> > > <john.mcnamara@intel.com>; dev@dpdk.org; Lu, Wenzhuo
> > > <wenzhuo.lu@intel.com>
> > > Subject: Re: [dpdk-dev] [PATCH v4 1/3] lib/librte_ether: add support
> > > for port reset
> > >
> > > 2017-04-14 01:29, Zhao1, Wei:
> > > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > > 2017-04-13 08:55, Zhao1, Wei:
> > > > > > From: Ananyev, Konstantin
> > > > > > > From: Zhao1, Wei
> > > > > > > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > > > > > > 2017-04-06 02:57, Zhao1, Wei:
> > > > > > > > > > > /**
> > > > > > > > > > > > + * Reset an ethernet device when it's not working.
> > > > > > > > > > > > + One scenario is, after PF
> > > > > > > > > > > > + * port is down and up, the related VF port
> > > > > > > > > > > > + should be
> > reset.
> > > > > > > > > > > > + * The API will stop the port, clear the rx/tx
> > > > > > > > > > > > + queues, re-setup the rx/tx
> > > > > > > > > > > > + * queues, restart the port.
> > > > > > > > > > >
> > > > > > > > > > > s/The API/This function/
> > > > > > > > > > >
> > > > > > > > > > > Please explain exactly the responsibility of this
> > > > > > > > > > > function, and how it is different from calling
> > > stop/configure/start.
> > > > > > > > > >
> > > > > > > > > > In this reset feature, reset function can do the
> > > > > > > > > > calling stop/configure/start process, but also It can
> > > > > > > > > > also do some restore work for the port, for example,
> > > > > > > > > > it can restore the added parameters of
> > > > > > > > > vlan, mac_addrs, promisc_unicast_enabled falg and
> > > > > > > > > promisc_multicast_enabled flag.
> > > > > > >
> > > > > > > Ok, but why start/stop can't do these things?
> > > > > > > Konstantin
> > > > > >
> > > > > > This is because in i40e PMD code, start and stop process do
> > > > > > not have the process of store and restore the added key
> parameters.
> > > > > > Not only i40e but also other PMD code. So, in the function
> > > > > > pointed to by dev_reset,
> > > > > we add specific function do store and restore of some of the
> > > > > important parameters listed above.
> > > > >
> > > > > Why store and restore cannot be implemented in start/stop functions?
> > > >
> > > > Because reset and start/stop are used for two purposes, for example:
> > > > Some user maybe just start/stop the port and he do not care what
> > > > key parameters has been configuration last time, and even worse
> > > > when he want to clear all the configuration last time , if we add
> > > > specific function do store and restore in that two function, it
> > > > is useless for them,
> > > and may cause a result that user do not expect.
> > >
> > > Is it said somewhere in the doc that the configuration is lost when
> > > stopping a device?
> > > Can we say which configuration parameter is kept and which one is lost?
> > >
> > > rte_eth_dev_config_restore() is called in rte_eth_dev_start().
> >
> > Port reset process not only involve the rte_eth_dev_start() and
> > rte_eth_dev_stop().
> > It also involve eth_dev_uninit() and eth_dev_init() process，
> > in which PMD device uninit and init. In this case, for example, data-
> > >mac_addrs buffer is freed so it need to add store and restore function.
> > BUT, if you only call stop/configure/start process, that is not
> > strictly what named "device reset".
On Thu, Apr 06, 2017 at 02:57:29AM +0000, Zhao1, Wei wrote:
> > > + * Reset an ethernet device when it's not working. One scenario is,
> > > + after PF
> > > + * port is down and up, the related VF port should be reset.
> > > + * The API will stop the port, clear the rx/tx queues, re-setup the
> > > + rx/tx
> > > + * queues, restart the port.
> >
> > s/The API/This function/
> >
> > Please explain exactly the responsibility of this function, and how it is
> > different from calling stop/configure/start.
>
> In this reset feature, reset function can do the calling stop/configure/start process, but also
> It can also do some restore work for the port, for example, it can restore the added parameters
> of vlan, mac_addrs, promisc_unicast_enabled falg and promisc_multicast_enabled flag.
> Maybe , I should add this explanation in the patch comments or function comments?
I'm curious why we have to do save & restore for a reset operation.
Why some configures have to be saved and restored? Doesn't "reset"
literally means reset everything?
Even though, how do you tell what kind of configures need be restored
and what should not? Again, even though, will all PMDs supports
restoring the same set of configurations?
While looking at your reset implementation for i40e, it looks more
complex than necessary: just thinking we have to call "xxx_queue_setup"
for all PMDs.
I'm thinking a simple hardware reset might be enough?
/* literally reset the hardware: reset everything */
rte_eth_reset(port)
{
eth_dev->ops->reset();
}
Assume the application already has a function (say, port_init()) to
initiate a specific port, it then just needs do something like following
to handle the case you described in the commit log:
rte_eth_reset(port);
port_init(port);
Makes sense? Sorry it's completely wrong; I've limited knowledge on NIC
pmd drivers after all :/
--yliu
Hi, Yuanhan
> -----Original Message-----
> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> Sent: Thursday, April 20, 2017 2:08 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Mcnamara, John
> <john.mcnamara@intel.com>; dev@dpdk.org; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>; Thomas Monjalon <thomas@monjalon.net>; Liu,
> Yu Y <yu.y.liu@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v4 1/3] lib/librte_ether: add support for port
> reset
>
> On Thu, Apr 06, 2017 at 02:57:29AM +0000, Zhao1, Wei wrote:
> > > > + * Reset an ethernet device when it's not working. One scenario
> > > > + is, after PF
> > > > + * port is down and up, the related VF port should be reset.
> > > > + * The API will stop the port, clear the rx/tx queues, re-setup
> > > > + the rx/tx
> > > > + * queues, restart the port.
> > >
> > > s/The API/This function/
> > >
> > > Please explain exactly the responsibility of this function, and how
> > > it is different from calling stop/configure/start.
> >
> > In this reset feature, reset function can do the calling
> > stop/configure/start process, but also It can also do some restore
> > work for the port, for example, it can restore the added parameters of
> vlan, mac_addrs, promisc_unicast_enabled falg and
> promisc_multicast_enabled flag.
> > Maybe , I should add this explanation in the patch comments or function
> comments?
>
> I'm curious why we have to do save & restore for a reset operation.
> Why some configures have to be saved and restored? Doesn't "reset"
> literally means reset everything?
>
Users maybe do not want to do a second configuration operation to waste time after reset which lost all previous configuration.
But he still want these configuration valid after reset.
So, save & restore can help them to save this process time and effort.
> Even though, how do you tell what kind of configures need be restored and
> what should not? Again, even though, will all PMDs supports restoring the
> same set of configurations?
>
Yes, this is hard to say what may be need and what may be not for user.
Now, the kinds of supported is list in patch set comment. And only i40e NIC support this feature.
> While looking at your reset implementation for i40e, it looks more complex
> than necessary: just thinking we have to call "xxx_queue_setup"
> for all PMDs.
>
> I'm thinking a simple hardware reset might be enough?
>
> /* literally reset the hardware: reset everything */
> rte_eth_reset(port)
> {
> eth_dev->ops->reset();
> }
>
You mean just do a reset and do not restore any configuration?
That may not meet the need for this feature from customer?
> Assume the application already has a function (say, port_init()) to initiate a
> specific port, it then just needs do something like following to handle the
> case you described in the commit log:
>
> rte_eth_reset(port);
> port_init(port);
>
You mean "rte_eth_reset" is the function of "rte_eth_dev_reset"?
I do not find any function named rte_eth_reset.
> Makes sense? Sorry it's completely wrong; I've limited knowledge on NIC
> pmd drivers after all :/
>
> --yliu
On Thu, Apr 20, 2017 at 09:17:24AM +0000, Zhao1, Wei wrote:
> > > > Please explain exactly the responsibility of this function, and how
> > > > it is different from calling stop/configure/start.
> > >
> > > In this reset feature, reset function can do the calling
> > > stop/configure/start process, but also It can also do some restore
> > > work for the port, for example, it can restore the added parameters of
> > vlan, mac_addrs, promisc_unicast_enabled falg and
> > promisc_multicast_enabled flag.
> > > Maybe , I should add this explanation in the patch comments or function
> > comments?
> >
> > I'm curious why we have to do save & restore for a reset operation.
> > Why some configures have to be saved and restored? Doesn't "reset"
> > literally means reset everything?
> >
>
> Users maybe do not want to do a second configuration operation to waste time after reset which lost all previous configuration.
> But he still want these configuration valid after reset.
> So, save & restore can help them to save this process time and effort.
>
> > Even though, how do you tell what kind of configures need be restored and
> > what should not? Again, even though, will all PMDs supports restoring the
> > same set of configurations?
> >
>
> Yes, this is hard to say what may be need and what may be not for user.
> Now, the kinds of supported is list in patch set comment. And only i40e NIC support this feature.
Why it's the configurations listed in patch 2? Because they are requested
by customers?
Is that all could be saved? If not, are you going to save & restore all
possible configurations?
Assuming the configurations saved & restored may differ from different
PMD drivers, how could you keep the consistency then? And judging that the
application has no idea about the difference (yet it knows nothing about
what kind of configurations will be saved), how could the application know
that some configurations are not saved & restored by the driver that it
has to do re-configuration by itself?
> > While looking at your reset implementation for i40e, it looks more complex
> > than necessary: just thinking we have to call "xxx_queue_setup"
> > for all PMDs.
> >
> > I'm thinking a simple hardware reset might be enough?
> >
> > /* literally reset the hardware: reset everything */
> > rte_eth_reset(port)
> > {
> > eth_dev->ops->reset();
> > }
> >
>
> You mean just do a reset and do not restore any configuration?
> That may not meet the need for this feature from customer?
Right, I'm just aware of the configuration might be done by PF (but not
only by the application), that the VF port may be not aware of those
configurations. So the save & restore is needed. I don't quite like
how it is done in this patch set though. I also don't think the API is
well named: as said, reset should literally reset everything.
We may need think how to do it properly.
Thomas, Konstantin, what do you guys think of it?
--yliu
21/04/2017 04:27, Yuanhan Liu:
> On Thu, Apr 20, 2017 at 09:17:24AM +0000, Zhao1, Wei wrote:
> > > > > Please explain exactly the responsibility of this function, and how
> > > > > it is different from calling stop/configure/start.
> > > >
> > > > In this reset feature, reset function can do the calling
> > > > stop/configure/start process, but also It can also do some restore
> > > > work for the port, for example, it can restore the added parameters
> > > > of
> > >
> > > vlan, mac_addrs, promisc_unicast_enabled falg and
> > > promisc_multicast_enabled flag.
> > >
> > > > Maybe , I should add this explanation in the patch comments or
> > > > function
> > >
> > > comments?
> > >
> > > I'm curious why we have to do save & restore for a reset operation.
> > > Why some configures have to be saved and restored? Doesn't "reset"
> > > literally means reset everything?
> >
> > Users maybe do not want to do a second configuration operation to waste
> > time after reset which lost all previous configuration. But he still want
> > these configuration valid after reset.
> > So, save & restore can help them to save this process time and effort.
> >
> > > Even though, how do you tell what kind of configures need be restored
> > > and
> > > what should not? Again, even though, will all PMDs supports restoring
> > > the
> > > same set of configurations?
> >
> > Yes, this is hard to say what may be need and what may be not for user.
> > Now, the kinds of supported is list in patch set comment. And only i40e
> > NIC support this feature.
> Why it's the configurations listed in patch 2? Because they are requested
> by customers?
>
> Is that all could be saved? If not, are you going to save & restore all
> possible configurations?
>
> Assuming the configurations saved & restored may differ from different
> PMD drivers, how could you keep the consistency then? And judging that the
> application has no idea about the difference (yet it knows nothing about
> what kind of configurations will be saved), how could the application know
> that some configurations are not saved & restored by the driver that it
> has to do re-configuration by itself?
>
> > > While looking at your reset implementation for i40e, it looks more
> > > complex
> > > than necessary: just thinking we have to call "xxx_queue_setup"
> > > for all PMDs.
> > >
> > > I'm thinking a simple hardware reset might be enough?
> > >
> > > /* literally reset the hardware: reset everything */
> > > rte_eth_reset(port)
> > > {
> > >
> > > eth_dev->ops->reset();
> > >
> > > }
> >
> > You mean just do a reset and do not restore any configuration?
> > That may not meet the need for this feature from customer?
>
> Right, I'm just aware of the configuration might be done by PF (but not
> only by the application), that the VF port may be not aware of those
> configurations. So the save & restore is needed. I don't quite like
> how it is done in this patch set though. I also don't think the API is
> well named: as said, reset should literally reset everything.
>
> We may need think how to do it properly.
>
> Thomas, Konstantin, what do you guys think of it?
I have the same concerns.
I think we should better document the current status of start/stop
and which configuration parameters are lost or saved.
> -----Original Message-----
> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> Sent: Friday, April 21, 2017 10:28 AM
> To: Zhao1, Wei <wei.zhao1@intel.com>
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Mcnamara, John
> <john.mcnamara@intel.com>; dev@dpdk.org; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>; Thomas Monjalon <thomas@monjalon.net>; Liu,
> Yu Y <yu.y.liu@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v4 1/3] lib/librte_ether: add support for port
> reset
>
> On Thu, Apr 20, 2017 at 09:17:24AM +0000, Zhao1, Wei wrote:
> > > > > Please explain exactly the responsibility of this function, and
> > > > > how it is different from calling stop/configure/start.
> > > >
> > > > In this reset feature, reset function can do the calling
> > > > stop/configure/start process, but also It can also do some restore
> > > > work for the port, for example, it can restore the added
> > > > parameters of
> > > vlan, mac_addrs, promisc_unicast_enabled falg and
> > > promisc_multicast_enabled flag.
> > > > Maybe , I should add this explanation in the patch comments or
> > > > function
> > > comments?
> > >
> > > I'm curious why we have to do save & restore for a reset operation.
> > > Why some configures have to be saved and restored? Doesn't "reset"
> > > literally means reset everything?
> > >
> >
> > Users maybe do not want to do a second configuration operation to waste
> time after reset which lost all previous configuration.
> > But he still want these configuration valid after reset.
> > So, save & restore can help them to save this process time and effort.
> >
> > > Even though, how do you tell what kind of configures need be
> > > restored and what should not? Again, even though, will all PMDs
> > > supports restoring the same set of configurations?
> > >
> >
> > Yes, this is hard to say what may be need and what may be not for user.
> > Now, the kinds of supported is list in patch set comment. And only i40e NIC
> support this feature.
>
> Why it's the configurations listed in patch 2? Because they are requested by
> customers?
>
> Is that all could be saved? If not, are you going to save & restore all possible
> configurations?
>
> Assuming the configurations saved & restored may differ from different
> PMD drivers, how could you keep the consistency then? And judging that the
> application has no idea about the difference (yet it knows nothing about
> what kind of configurations will be saved), how could the application know
> that some configurations are not saved & restored by the driver that it has to
> do re-configuration by itself?
>
Good idea, so maybe I should add some words in doc\guides\nics\i40e.rst to
Record which configurations are saved and restored by the PMD driver in reset function.
Which not list in that are recognized as not saved and restored by default.
Is that ok ?
> > > While looking at your reset implementation for i40e, it looks more
> > > complex than necessary: just thinking we have to call
> "xxx_queue_setup"
> > > for all PMDs.
> > >
> > > I'm thinking a simple hardware reset might be enough?
> > >
> > > /* literally reset the hardware: reset everything */
> > > rte_eth_reset(port)
> > > {
> > > eth_dev->ops->reset();
> > > }
> > >
> >
> > You mean just do a reset and do not restore any configuration?
> > That may not meet the need for this feature from customer?
>
> Right, I'm just aware of the configuration might be done by PF (but not only
> by the application), that the VF port may be not aware of those
> configurations. So the save & restore is needed. I don't quite like how it is
> done in this patch set though. I also don't think the API is well named: as said,
> reset should literally reset everything.
>
> We may need think how to do it properly.
>
> Thomas, Konstantin, what do you guys think of it?
So, your opinion is if it is named "reset", we had better do not do any restore work?
If we keep this restore feature, we had better change function name?
May be use the name "reset_and_restore" as function and feature name ?
>
> --yliu
Hi, thomas
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Friday, April 21, 2017 4:28 PM
> To: Yuanhan Liu <yuanhan.liu@linux.intel.com>; Zhao1, Wei
> <wei.zhao1@intel.com>
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Mcnamara, John
> <john.mcnamara@intel.com>; dev@dpdk.org; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>; Liu, Yu Y <yu.y.liu@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v4 1/3] lib/librte_ether: add support for port
> reset
>
> 21/04/2017 04:27, Yuanhan Liu:
> > On Thu, Apr 20, 2017 at 09:17:24AM +0000, Zhao1, Wei wrote:
> > > > > > Please explain exactly the responsibility of this function,
> > > > > > and how it is different from calling stop/configure/start.
> > > > >
> > > > > In this reset feature, reset function can do the calling
> > > > > stop/configure/start process, but also It can also do some
> > > > > restore work for the port, for example, it can restore the added
> > > > > parameters of
> > > >
> > > > vlan, mac_addrs, promisc_unicast_enabled falg and
> > > > promisc_multicast_enabled flag.
> > > >
> > > > > Maybe , I should add this explanation in the patch comments or
> > > > > function
> > > >
> > > > comments?
> > > >
> > > > I'm curious why we have to do save & restore for a reset operation.
> > > > Why some configures have to be saved and restored? Doesn't "reset"
> > > > literally means reset everything?
> > >
> > > Users maybe do not want to do a second configuration operation to
> > > waste time after reset which lost all previous configuration. But he
> > > still want these configuration valid after reset.
> > > So, save & restore can help them to save this process time and effort.
> > >
> > > > Even though, how do you tell what kind of configures need be
> > > > restored and what should not? Again, even though, will all PMDs
> > > > supports restoring the same set of configurations?
> > >
> > > Yes, this is hard to say what may be need and what may be not for user.
> > > Now, the kinds of supported is list in patch set comment. And only
> > > i40e NIC support this feature.
> > Why it's the configurations listed in patch 2? Because they are
> > requested by customers?
> >
> > Is that all could be saved? If not, are you going to save & restore
> > all possible configurations?
> >
> > Assuming the configurations saved & restored may differ from different
> > PMD drivers, how could you keep the consistency then? And judging that
> > the application has no idea about the difference (yet it knows nothing
> > about what kind of configurations will be saved), how could the
> > application know that some configurations are not saved & restored by
> > the driver that it has to do re-configuration by itself?
> >
> > > > While looking at your reset implementation for i40e, it looks more
> > > > complex than necessary: just thinking we have to call
> > > > "xxx_queue_setup"
> > > > for all PMDs.
> > > >
> > > > I'm thinking a simple hardware reset might be enough?
> > > >
> > > > /* literally reset the hardware: reset everything */
> > > > rte_eth_reset(port)
> > > > {
> > > >
> > > > eth_dev->ops->reset();
> > > >
> > > > }
> > >
> > > You mean just do a reset and do not restore any configuration?
> > > That may not meet the need for this feature from customer?
> >
> > Right, I'm just aware of the configuration might be done by PF (but
> > not only by the application), that the VF port may be not aware of
> > those configurations. So the save & restore is needed. I don't quite
> > like how it is done in this patch set though. I also don't think the
> > API is well named: as said, reset should literally reset everything.
> >
> > We may need think how to do it properly.
> >
> > Thomas, Konstantin, what do you guys think of it?
>
> I have the same concerns.
> I think we should better document the current status of start/stop and which
> configuration parameters are lost or saved.
Maybe I should add some words in doc\guides\nics\i40e.rst to Record which configurations are saved and restored by the PMD driver in reset function.
Which not list in that are recognized as not saved and restored by default.
OTHER NIC for this feature can add similar record in their xxx.rst.
21/04/2017 10:59, Zhao1, Wei:
> Hi, thomas
>
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 21/04/2017 04:27, Yuanhan Liu:
> > > On Thu, Apr 20, 2017 at 09:17:24AM +0000, Zhao1, Wei wrote:
> > > > > > > Please explain exactly the responsibility of this function,
> > > > > > > and how it is different from calling stop/configure/start.
> > > > > >
> > > > > > In this reset feature, reset function can do the calling
> > > > > > stop/configure/start process, but also It can also do some
> > > > > > restore work for the port, for example, it can restore the added
> > > > > > parameters of
> > > > >
> > > > > vlan, mac_addrs, promisc_unicast_enabled falg and
> > > > > promisc_multicast_enabled flag.
> > > > >
> > > > > > Maybe , I should add this explanation in the patch comments or
> > > > > > function
> > > > >
> > > > > comments?
> > > > >
> > > > > I'm curious why we have to do save & restore for a reset operation.
> > > > > Why some configures have to be saved and restored? Doesn't "reset"
> > > > > literally means reset everything?
> > > >
> > > > Users maybe do not want to do a second configuration operation to
> > > > waste time after reset which lost all previous configuration. But he
> > > > still want these configuration valid after reset.
> > > > So, save & restore can help them to save this process time and effort.
> > > >
> > > > > Even though, how do you tell what kind of configures need be
> > > > > restored and what should not? Again, even though, will all PMDs
> > > > > supports restoring the same set of configurations?
> > > >
> > > > Yes, this is hard to say what may be need and what may be not for
> > > > user.
> > > > Now, the kinds of supported is list in patch set comment. And only
> > > > i40e NIC support this feature.
> > >
> > > Why it's the configurations listed in patch 2? Because they are
> > > requested by customers?
> > >
> > > Is that all could be saved? If not, are you going to save & restore
> > > all possible configurations?
> > >
> > > Assuming the configurations saved & restored may differ from different
> > > PMD drivers, how could you keep the consistency then? And judging that
> > > the application has no idea about the difference (yet it knows nothing
> > > about what kind of configurations will be saved), how could the
> > > application know that some configurations are not saved & restored by
> > > the driver that it has to do re-configuration by itself?
> > >
> > > > > While looking at your reset implementation for i40e, it looks more
> > > > > complex than necessary: just thinking we have to call
> > > > > "xxx_queue_setup"
> > > > > for all PMDs.
> > > > >
> > > > > I'm thinking a simple hardware reset might be enough?
> > > > >
> > > > > /* literally reset the hardware: reset everything */
> > > > > rte_eth_reset(port)
> > > > > {
> > > > >
> > > > > eth_dev->ops->reset();
> > > > >
> > > > > }
> > > >
> > > > You mean just do a reset and do not restore any configuration?
> > > > That may not meet the need for this feature from customer?
> > >
> > > Right, I'm just aware of the configuration might be done by PF (but
> > > not only by the application), that the VF port may be not aware of
> > > those configurations. So the save & restore is needed. I don't quite
> > > like how it is done in this patch set though. I also don't think the
> > > API is well named: as said, reset should literally reset everything.
> > >
> > > We may need think how to do it properly.
> > >
> > > Thomas, Konstantin, what do you guys think of it?
> >
> > I have the same concerns.
> > I think we should better document the current status of start/stop and
> > which configuration parameters are lost or saved.
>
> Maybe I should add some words in doc\guides\nics\i40e.rst to Record which
> configurations are saved and restored by the PMD driver in reset
> function. Which not list in that are recognized as not saved and restored
> by default. OTHER NIC for this feature can add similar record in their
> xxx.rst.
No, when defining a generic API in ethdev, we must define the same
behaviour for every drivers.
Please check how to make the behaviour consistent and documented
in ethdev. We may need to document your new function and start/stop also.
On Fri, Apr 21, 2017 at 11:28:37AM +0200, Thomas Monjalon wrote:
> > Maybe I should add some words in doc\guides\nics\i40e.rst to Record which
> > configurations are saved and restored by the PMD driver in reset
> > function. Which not list in that are recognized as not saved and restored
> > by default. OTHER NIC for this feature can add similar record in their
> > xxx.rst.
>
> No, when defining a generic API in ethdev, we must define the same
> behaviour for every drivers.
Agreed. That was my point.
--yliu
> Please check how to make the behaviour consistent and documented
> in ethdev. We may need to document your new function and start/stop also.
Hi, yuanhan & thomas
> -----Original Message-----
> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> Sent: Monday, April 24, 2017 10:01 AM
> To: Thomas Monjalon <thomas@monjalon.net>
> Cc: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org; Ananyev,
> Konstantin <konstantin.ananyev@intel.com>; Mcnamara, John
> <john.mcnamara@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Liu,
> Yu Y <yu.y.liu@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v4 1/3] lib/librte_ether: add support for port
> reset
>
> On Fri, Apr 21, 2017 at 11:28:37AM +0200, Thomas Monjalon wrote:
> > > Maybe I should add some words in doc\guides\nics\i40e.rst to Record
> > > which configurations are saved and restored by the PMD driver in
> > > reset function. Which not list in that are recognized as not saved
> > > and restored by default. OTHER NIC for this feature can add similar
> > > record in their xxx.rst.
> >
> > No, when defining a generic API in ethdev, we must define the same
> > behaviour for every drivers.
>
> Agreed. That was my point.
>
> --yliu
>
> > Please check how to make the behaviour consistent and documented in
> > ethdev. We may need to document your new function and start/stop also.
Maybe I should record these reset and restore info in some doc in rte layer.
And we have only implement this feature in i40e NIC by now.
Hi, yuanhan & thomas
> -----Original Message-----
> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> Sent: Monday, April 24, 2017 10:01 AM
> To: Thomas Monjalon <thomas@monjalon.net>
> Cc: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org; Ananyev,
> Konstantin <konstantin.ananyev@intel.com>; Mcnamara, John
> <john.mcnamara@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Liu,
> Yu Y <yu.y.liu@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v4 1/3] lib/librte_ether: add support for port
> reset
>
> On Fri, Apr 21, 2017 at 11:28:37AM +0200, Thomas Monjalon wrote:
> > > Maybe I should add some words in doc\guides\nics\i40e.rst to Record
> > > which configurations are saved and restored by the PMD driver in
> > > reset function. Which not list in that are recognized as not saved
> > > and restored by default. OTHER NIC for this feature can add similar
> > > record in their xxx.rst.
> >
> > No, when defining a generic API in ethdev, we must define the same
> > behaviour for every drivers.
>
> Agreed. That was my point.
>
> --yliu
>
> > Please check how to make the behaviour consistent and documented in
> > ethdev. We may need to document your new function and start/stop also.
Do you have any suggestion on which document in rte layer to record store and restore info by me?
24/04/2017 05:39, Zhao1, Wei:
> Hi, yuanhan & thomas
> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> > On Fri, Apr 21, 2017 at 11:28:37AM +0200, Thomas Monjalon wrote:
> > > > Maybe I should add some words in doc\guides\nics\i40e.rst to Record
> > > > which configurations are saved and restored by the PMD driver in
> > > > reset function. Which not list in that are recognized as not saved
> > > > and restored by default. OTHER NIC for this feature can add similar
> > > > record in their xxx.rst.
> > >
> > > No, when defining a generic API in ethdev, we must define the same
> > > behaviour for every drivers.
> >
> > Agreed. That was my point.
> >
> > --yliu
> >
> > > Please check how to make the behaviour consistent and documented in
> > > ethdev. We may need to document your new function and start/stop also.
>
> Do you have any suggestion on which document in rte layer to record store
> and restore info by me?
It should be documented in the doxygen comment of the functions.
Either we explain which configuration is restored on start and reset,
or we state everything (or nothing) is restored except the configurations
commented in the related configuration functions.
Ok.
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Monday, April 24, 2017 4:05 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>
> Cc: dev@dpdk.org; Yuanhan Liu <yuanhan.liu@linux.intel.com>; Ananyev,
> Konstantin <konstantin.ananyev@intel.com>; Mcnamara, John
> <john.mcnamara@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Liu,
> Yu Y <yu.y.liu@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v4 1/3] lib/librte_ether: add support for port
> reset
>
> 24/04/2017 05:39, Zhao1, Wei:
> > Hi, yuanhan & thomas
> > From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> > > On Fri, Apr 21, 2017 at 11:28:37AM +0200, Thomas Monjalon wrote:
> > > > > Maybe I should add some words in doc\guides\nics\i40e.rst to
> > > > > Record which configurations are saved and restored by the PMD
> > > > > driver in reset function. Which not list in that are recognized
> > > > > as not saved and restored by default. OTHER NIC for this
> > > > > feature can add similar record in their xxx.rst.
> > > >
> > > > No, when defining a generic API in ethdev, we must define the same
> > > > behaviour for every drivers.
> > >
> > > Agreed. That was my point.
> > >
> > > --yliu
> > >
> > > > Please check how to make the behaviour consistent and documented
> > > > in ethdev. We may need to document your new function and start/stop
> also.
> >
> > Do you have any suggestion on which document in rte layer to record
> > store and restore info by me?
>
> It should be documented in the doxygen comment of the functions.
> Either we explain which configuration is restored on start and reset, or we
> state everything (or nothing) is restored except the configurations
> commented in the related configuration functions.
@@ -3273,3 +3273,20 @@ rte_eth_dev_l2_tunnel_offload_set(uint8_t port_id,
-ENOTSUP);
return (*dev->dev_ops->l2_tunnel_offload_set)(dev, l2_tunnel, mask, en);
}
+
+int
+rte_eth_dev_reset(uint8_t port_id)
+{
+ struct rte_eth_dev *dev;
+ int diag;
+
+ RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+
+ dev = &rte_eth_devices[port_id];
+
+ RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_reset, -ENOTSUP);
+
+ diag = (*dev->dev_ops->dev_reset)(dev);
+
+ return diag;
+}
@@ -1367,6 +1367,9 @@ typedef int (*eth_l2_tunnel_offload_set_t)
uint8_t en);
/**< @internal enable/disable the l2 tunnel offload functions */
+typedef int (*eth_dev_reset_t)(struct rte_eth_dev *dev);
+/**< @internal Function used to reset a configured Ethernet device. */
+
#ifdef RTE_NIC_BYPASS
enum {
@@ -1509,6 +1512,9 @@ struct eth_dev_ops {
eth_l2_tunnel_offload_set_t l2_tunnel_offload_set;
/** Enable/disable l2 tunnel offload functions. */
+ /** Reset device. */
+ eth_dev_reset_t dev_reset;
+
eth_set_queue_rate_limit_t set_queue_rate_limit; /**< Set queue rate limit. */
rss_hash_update_t rss_hash_update; /** Configure RSS hash protocols. */
@@ -4413,6 +4419,25 @@ int
rte_eth_dev_get_name_by_port(uint8_t port_id, char *name);
/**
+ * Reset an ethernet device when it's not working. One scenario is, after PF
+ * port is down and up, the related VF port should be reset.
+ * The API will stop the port, clear the rx/tx queues, re-setup the rx/tx
+ * queues, restart the port.
+ * Before calling this API, APP should stop the rx/tx. When tx is being stopped,
+ * APP can drop the packets and release the buffer instead of sending them.
+ *
+ * @param port_id
+ * The port identifier of the Ethernet device.
+ *
+ * @return
+ * - (0) if successful.
+ * - (-ENODEV) if port identifier is invalid.
+ * - (-ENOTSUP) if hardware doesn't support this function.
+ */
+int
+rte_eth_dev_reset(uint8_t port_id);
+
+/**
* @internal
* Wrapper for use by pci drivers as a .probe function to attach to a ethdev
* interface.
@@ -154,3 +154,9 @@ DPDK_17.02 {
rte_flow_validate;
} DPDK_16.11;
+
+DPDK_17.05 {
+ global:
+
+ rte_eth_dev_reset;
+} DPDK_17.02;
\ No newline at end of file