[V4,8/9] app/testpmd: show example to handle hot unplug
Checks
Commit Message
Use testpmd for example, to show how an application smoothly handle
failure when device being hot unplug. If app have enabled the device event
monitor and register the hot plug event’s callback before running, once
app detect the removal event, the callback would be called. It will first
stop the packet forwarding, then stop the port, close the port, and finally
detach the port to remove the device out from the device lists.
Signed-off-by: Jeff Guo <jia.guo@intel.com>
---
v4->v3:
remove some unused code
---
app/test-pmd/testpmd.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
Comments
Hi Jeff
A good advance, thank you, but as I said in previous version, this patch inserts a bug and the next one fixes it.
Patch 9 should be before patch 8 while this patch just add 1 more option for EAL hotplug.
Please see 1 more comment below.
From: Jeff Guo
> Use testpmd for example, to show how an application smoothly handle failure
> when device being hot unplug. If app have enabled the device event monitor
> and register the hot plug event’s callback before running, once app detect the
> removal event, the callback would be called. It will first stop the packet
> forwarding, then stop the port, close the port, and finally detach the port to
> remove the device out from the device lists.
>
> Signed-off-by: Jeff Guo <jia.guo@intel.com>
> ---
> v4->v3:
> remove some unused code
> ---
> app/test-pmd/testpmd.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> 24c1998..42ed196 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -2196,6 +2196,9 @@ static void
> eth_dev_event_callback(char *device_name, enum rte_dev_event_type type,
> __rte_unused void *arg)
> {
> + uint16_t port_id;
> + int ret;
> +
> if (type >= RTE_DEV_EVENT_MAX) {
> fprintf(stderr, "%s called upon invalid event %d\n",
> __func__, type);
> @@ -2206,9 +2209,12 @@ eth_dev_event_callback(char *device_name, enum
> rte_dev_event_type type,
> case RTE_DEV_EVENT_REMOVE:
> RTE_LOG(ERR, EAL, "The device: %s has been removed!\n",
> device_name);
> - /* TODO: After finish failure handle, begin to stop
> - * packet forward, stop port, close port, detach port.
> - */
> + ret = rte_eth_dev_get_port_by_name(device_name, &port_id);
As you probably know, 1 rte_device may be associated to more than one ethdev ports, so the ethdev port name can be different from rte_device name.
Looks like we need a new ethdev API to get all the ports associated to one rte_device.
> + if (ret) {
> + printf("can not get port by device %s!\n",
> device_name);
> + return;
> + }
> + rmv_event_callback((void *)(intptr_t)port_id);
> break;
> case RTE_DEV_EVENT_ADD:
> RTE_LOG(ERR, EAL, "The device: %s has been added!\n", @@ -
> 2736,7 +2742,6 @@ main(int argc, char** argv)
> return -1;
> }
> eth_dev_event_callback_register();
> -
> }
>
> if (start_port(RTE_PORT_ALL) != 0)
> --
> 2.7.4
mantan,
On 7/1/2018 3:46 PM, Matan Azrad wrote:
> Hi Jeff
>
> A good advance, thank you, but as I said in previous version, this patch inserts a bug and the next one fixes it.
> Patch 9 should be before patch 8 while this patch just add 1 more option for EAL hotplug.
i agree that patch 9 before patch 8 could be better. thank.
> Please see 1 more comment below.
>
> From: Jeff Guo
>> Use testpmd for example, to show how an application smoothly handle failure
>> when device being hot unplug. If app have enabled the device event monitor
>> and register the hot plug event’s callback before running, once app detect the
>> removal event, the callback would be called. It will first stop the packet
>> forwarding, then stop the port, close the port, and finally detach the port to
>> remove the device out from the device lists.
>>
>> Signed-off-by: Jeff Guo <jia.guo@intel.com>
>> ---
>> v4->v3:
>> remove some unused code
>> ---
>> app/test-pmd/testpmd.c | 13 +++++++++----
>> 1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>> 24c1998..42ed196 100644
>> --- a/app/test-pmd/testpmd.c
>> +++ b/app/test-pmd/testpmd.c
>> @@ -2196,6 +2196,9 @@ static void
>> eth_dev_event_callback(char *device_name, enum rte_dev_event_type type,
>> __rte_unused void *arg)
>> {
>> + uint16_t port_id;
>> + int ret;
>> +
>> if (type >= RTE_DEV_EVENT_MAX) {
>> fprintf(stderr, "%s called upon invalid event %d\n",
>> __func__, type);
>> @@ -2206,9 +2209,12 @@ eth_dev_event_callback(char *device_name, enum
>> rte_dev_event_type type,
>> case RTE_DEV_EVENT_REMOVE:
>> RTE_LOG(ERR, EAL, "The device: %s has been removed!\n",
>> device_name);
>> - /* TODO: After finish failure handle, begin to stop
>> - * packet forward, stop port, close port, detach port.
>> - */
>> + ret = rte_eth_dev_get_port_by_name(device_name, &port_id);
> As you probably know, 1 rte_device may be associated to more than one ethdev ports, so the ethdev port name can be different from rte_device name.
> Looks like we need a new ethdev API to get all the ports associated to one rte_device.
agree, seems that the the old ethdev API have some issue when got all
port by device name. we could check with ethdev maintainer and fix it by
specific ethdev patch later.
>> + if (ret) {
>> + printf("can not get port by device %s!\n",
>> device_name);
>> + return;
>> + }
>> + rmv_event_callback((void *)(intptr_t)port_id);
>> break;
>> case RTE_DEV_EVENT_ADD:
>> RTE_LOG(ERR, EAL, "The device: %s has been added!\n", @@ -
>> 2736,7 +2742,6 @@ main(int argc, char** argv)
>> return -1;
>> }
>> eth_dev_event_callback_register();
>> -
>> }
>>
>> if (start_port(RTE_PORT_ALL) != 0)
>> --
>> 2.7.4
03/07/2018 11:35, Guo, Jia:
> On 7/1/2018 3:46 PM, Matan Azrad wrote:
> > From: Jeff Guo
> >> --- a/app/test-pmd/testpmd.c
> >> +++ b/app/test-pmd/testpmd.c
> >> @@ -2206,9 +2209,12 @@ eth_dev_event_callback(char *device_name, enum
> >> rte_dev_event_type type,
> >> case RTE_DEV_EVENT_REMOVE:
> >> RTE_LOG(ERR, EAL, "The device: %s has been removed!\n",
> >> device_name);
> >> - /* TODO: After finish failure handle, begin to stop
> >> - * packet forward, stop port, close port, detach port.
> >> - */
> >> + ret = rte_eth_dev_get_port_by_name(device_name, &port_id);
> > As you probably know, 1 rte_device may be associated to more than one ethdev ports, so the ethdev port name can be different from rte_device name.
> > Looks like we need a new ethdev API to get all the ports associated to one rte_device.
>
> agree, seems that the the old ethdev API have some issue when got all
> port by device name. we could check with ethdev maintainer and fix it by
> specific ethdev patch later.
This ethdev function could return an error if several ports match.
Ideally, we should not use this function at all.
If you want to manage an ethdev port, why are you using an EAL event?
There is an ethdev callback mechanism for port removal.
hi, thomas
On 7/4/2018 6:44 AM, Thomas Monjalon wrote:
> 03/07/2018 11:35, Guo, Jia:
>> On 7/1/2018 3:46 PM, Matan Azrad wrote:
>>> From: Jeff Guo
>>>> --- a/app/test-pmd/testpmd.c
>>>> +++ b/app/test-pmd/testpmd.c
>>>> @@ -2206,9 +2209,12 @@ eth_dev_event_callback(char *device_name, enum
>>>> rte_dev_event_type type,
>>>> case RTE_DEV_EVENT_REMOVE:
>>>> RTE_LOG(ERR, EAL, "The device: %s has been removed!\n",
>>>> device_name);
>>>> - /* TODO: After finish failure handle, begin to stop
>>>> - * packet forward, stop port, close port, detach port.
>>>> - */
>>>> + ret = rte_eth_dev_get_port_by_name(device_name, &port_id);
>>> As you probably know, 1 rte_device may be associated to more than one ethdev ports, so the ethdev port name can be different from rte_device name.
>>> Looks like we need a new ethdev API to get all the ports associated to one rte_device.
>> agree, seems that the the old ethdev API have some issue when got all
>> port by device name. we could check with ethdev maintainer and fix it by
>> specific ethdev patch later.
> This ethdev function could return an error if several ports match.
>
> Ideally, we should not use this function at all.
> If you want to manage an ethdev port, why are you using an EAL event?
> There is an ethdev callback mechanism for port removal.
>
>
i think the problem is that how to manage all ethdev port associated
with one rte_device. So the easy way is let device event callback to
check these ports. I will modify it in next version.
Hi Thomas, Guo
From: Thomas Monjalon
> 03/07/2018 11:35, Guo, Jia:
> > On 7/1/2018 3:46 PM, Matan Azrad wrote:
> > > From: Jeff Guo
> > >> --- a/app/test-pmd/testpmd.c
> > >> +++ b/app/test-pmd/testpmd.c
> > >> @@ -2206,9 +2209,12 @@ eth_dev_event_callback(char
> *device_name,
> > >> enum rte_dev_event_type type,
> > >> case RTE_DEV_EVENT_REMOVE:
> > >> RTE_LOG(ERR, EAL, "The device: %s has been removed!\n",
> > >> device_name);
> > >> - /* TODO: After finish failure handle, begin to stop
> > >> - * packet forward, stop port, close port, detach port.
> > >> - */
> > >> + ret = rte_eth_dev_get_port_by_name(device_name,
> &port_id);
> > > As you probably know, 1 rte_device may be associated to more than one
> ethdev ports, so the ethdev port name can be different from rte_device
> name.
> > > Looks like we need a new ethdev API to get all the ports associated to
> one rte_device.
> >
> > agree, seems that the the old ethdev API have some issue when got all
> > port by device name. we could check with ethdev maintainer and fix it
> > by specific ethdev patch later.
>
> This ethdev function could return an error if several ports match.
>
Just to clarify:
The ethdev name may be different from the rte_device name of a port,
The rte_eth_dev_get_port_by_name() searches the ethdev name and not the rte_device name.
> Ideally, we should not use this function at all.
> If you want to manage an ethdev port, why are you using an EAL event?
> There is an ethdev callback mechanism for port removal.
So, looks like the EAL event should trigger an ethdev event for all the ports associated to this rte_device.
I think that the best one to do it is the PMD, so maybe the PMD(which wants to support hot unplug) should register to the EAL event and to trigger an ethdev RMV event from the EAL callback.
What do you think?
On 7/4/2018 3:06 PM, Matan Azrad wrote:
> Hi Thomas, Guo
>
> From: Thomas Monjalon
>> 03/07/2018 11:35, Guo, Jia:
>>> On 7/1/2018 3:46 PM, Matan Azrad wrote:
>>>> From: Jeff Guo
>>>>> --- a/app/test-pmd/testpmd.c
>>>>> +++ b/app/test-pmd/testpmd.c
>>>>> @@ -2206,9 +2209,12 @@ eth_dev_event_callback(char
>> *device_name,
>>>>> enum rte_dev_event_type type,
>>>>> case RTE_DEV_EVENT_REMOVE:
>>>>> RTE_LOG(ERR, EAL, "The device: %s has been removed!\n",
>>>>> device_name);
>>>>> - /* TODO: After finish failure handle, begin to stop
>>>>> - * packet forward, stop port, close port, detach port.
>>>>> - */
>>>>> + ret = rte_eth_dev_get_port_by_name(device_name,
>> &port_id);
>>>> As you probably know, 1 rte_device may be associated to more than one
>> ethdev ports, so the ethdev port name can be different from rte_device
>> name.
>>>> Looks like we need a new ethdev API to get all the ports associated to
>> one rte_device.
>>> agree, seems that the the old ethdev API have some issue when got all
>>> port by device name. we could check with ethdev maintainer and fix it
>>> by specific ethdev patch later.
>> This ethdev function could return an error if several ports match.
>>
> Just to clarify:
>
> The ethdev name may be different from the rte_device name of a port,
> The rte_eth_dev_get_port_by_name() searches the ethdev name and not the rte_device name.
>
>> Ideally, we should not use this function at all.
>> If you want to manage an ethdev port, why are you using an EAL event?
>> There is an ethdev callback mechanism for port removal.
> So, looks like the EAL event should trigger an ethdev event for all the ports associated to this rte_device.
> I think that the best one to do it is the PMD, so maybe the PMD(which wants to support hot unplug) should register to the EAL event and to trigger an ethdev RMV event from the EAL callback.
>
> What do you think?
>
i think matan give an constructive option to combine the usage of eal
event and ethdev event, but i am not sure which is the best one.
So let this discuss on going, i will remove this 8/9 and 9/9 patches,
let the patch set focus on the hotplug failure mechanism ,
and will use another patch set to cover the event management example in
testpmd.
>
>
@@ -2196,6 +2196,9 @@ static void
eth_dev_event_callback(char *device_name, enum rte_dev_event_type type,
__rte_unused void *arg)
{
+ uint16_t port_id;
+ int ret;
+
if (type >= RTE_DEV_EVENT_MAX) {
fprintf(stderr, "%s called upon invalid event %d\n",
__func__, type);
@@ -2206,9 +2209,12 @@ eth_dev_event_callback(char *device_name, enum rte_dev_event_type type,
case RTE_DEV_EVENT_REMOVE:
RTE_LOG(ERR, EAL, "The device: %s has been removed!\n",
device_name);
- /* TODO: After finish failure handle, begin to stop
- * packet forward, stop port, close port, detach port.
- */
+ ret = rte_eth_dev_get_port_by_name(device_name, &port_id);
+ if (ret) {
+ printf("can not get port by device %s!\n", device_name);
+ return;
+ }
+ rmv_event_callback((void *)(intptr_t)port_id);
break;
case RTE_DEV_EVENT_ADD:
RTE_LOG(ERR, EAL, "The device: %s has been added!\n",
@@ -2736,7 +2742,6 @@ main(int argc, char** argv)
return -1;
}
eth_dev_event_callback_register();
-
}
if (start_port(RTE_PORT_ALL) != 0)