diff mbox series

[V2,3/6] ethdev: fix push new event

Message ID 20220915124522.5407-4-lihuisong@huawei.com (mailing list archive)
State Rejected, archived
Delegated to: Thomas Monjalon
Headers
Series app/testpmd: support attach and detach port for MP |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

lihuisong (C) Sept. 15, 2022, 12:45 p.m. UTC 
  The 'state' in struct rte_eth_dev may be used to update some information
when app receive these events. For example, when app receives a new event,
app may get the socket id of this port by calling rte_eth_dev_socket_id to
setup the attached port. The 'state' is used in rte_eth_dev_socket_id.

If the state isn't modified to RTE_ETH_DEV_ATTACHED before pushing the new
event, app will get the socket id failed. So this patch moves pushing event
operation after the state updated.

Fixes: 99a2dd955fba ("lib: remove librte_ prefix from directory names")
Cc: stable@dpdk.org

Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
 lib/ethdev/ethdev_driver.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Thomas Monjalon Sept. 27, 2022, 10:49 a.m. UTC | #1 
15/09/2022 14:45, Huisong Li:
> The 'state' in struct rte_eth_dev may be used to update some information
> when app receive these events. For example, when app receives a new event,
> app may get the socket id of this port by calling rte_eth_dev_socket_id to
> setup the attached port. The 'state' is used in rte_eth_dev_socket_id.
> 
> If the state isn't modified to RTE_ETH_DEV_ATTACHED before pushing the new
> event, app will get the socket id failed. So this patch moves pushing event
> operation after the state updated.
> 
> Fixes: 99a2dd955fba ("lib: remove librte_ prefix from directory names")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> ---
>  lib/ethdev/ethdev_driver.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/ethdev/ethdev_driver.c b/lib/ethdev/ethdev_driver.c
> index a285f213f0..a6616f072b 100644
> --- a/lib/ethdev/ethdev_driver.c
> +++ b/lib/ethdev/ethdev_driver.c
> @@ -206,9 +206,9 @@ rte_eth_dev_probing_finish(struct rte_eth_dev *dev)
>  	if (rte_eal_process_type() == RTE_PROC_SECONDARY)
>  		eth_dev_fp_ops_setup(rte_eth_fp_ops + dev->data->port_id, dev);
>  
> -	rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_NEW, NULL);
>  
>  	dev->state = RTE_ETH_DEV_ATTACHED;
> +	rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_NEW, NULL);
>  }

As explained in the first patch, I don't think it is a good solution.
We should not allow the port to be used until the end of probing.
When RTE_ETH_EVENT_NEW is sent, the device is allocated but
not ready for use. If an entity like failsafe decides to take ownership
of the port, then the application should not consider it at all.
For these reasons, we should limit which operations can be done
during RTE_ETH_EVENT_NEW processing.
That's why I've proposed creating a new state RTE_ETH_DEV_ALLOCATED,
not sure why you didn't follow this advice.
lihuisong (C) Oct. 8, 2022, 4:09 a.m. UTC | #2 
在 2022/9/27 18:49, Thomas Monjalon 写道:
> 15/09/2022 14:45, Huisong Li:
>> The 'state' in struct rte_eth_dev may be used to update some information
>> when app receive these events. For example, when app receives a new event,
>> app may get the socket id of this port by calling rte_eth_dev_socket_id to
>> setup the attached port. The 'state' is used in rte_eth_dev_socket_id.
>>
>> If the state isn't modified to RTE_ETH_DEV_ATTACHED before pushing the new
>> event, app will get the socket id failed. So this patch moves pushing event
>> operation after the state updated.
>>
>> Fixes: 99a2dd955fba ("lib: remove librte_ prefix from directory names")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> ---
>>   lib/ethdev/ethdev_driver.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/lib/ethdev/ethdev_driver.c b/lib/ethdev/ethdev_driver.c
>> index a285f213f0..a6616f072b 100644
>> --- a/lib/ethdev/ethdev_driver.c
>> +++ b/lib/ethdev/ethdev_driver.c
>> @@ -206,9 +206,9 @@ rte_eth_dev_probing_finish(struct rte_eth_dev *dev)
>>   	if (rte_eal_process_type() == RTE_PROC_SECONDARY)
>>   		eth_dev_fp_ops_setup(rte_eth_fp_ops + dev->data->port_id, dev);
>>   
>> -	rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_NEW, NULL);
>>   
>>   	dev->state = RTE_ETH_DEV_ATTACHED;
>> +	rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_NEW, NULL);
>>   }
> As explained in the first patch, I don't think it is a good solution.
> We should not allow the port to be used until the end of probing.
> When RTE_ETH_EVENT_NEW is sent, the device is allocated but
> not ready for use. If an entity like failsafe decides to take ownership
> of the port, then the application should not consider it at all.
> For these reasons, we should limit which operations can be done
> during RTE_ETH_EVENT_NEW processing.
> That's why I've proposed creating a new state RTE_ETH_DEV_ALLOCATED,
> not sure why you didn't follow this advice.
I want to put this problem to this case this patchset mentions, so as to
have a better discussion. From the first patch, I know what you mean.
But I still have some confusion:
When a device taken by failsafe PMD push new event, all event callback
will be called under this device. As you suggested, the device is a valid
port if its state is ALLOCATED or ATTACHED. Now, the macro 
RTE_ETH_FOREACH_DEV
uses the condition that device is valid(state is ALLOCATED or ATTACHED)
and NOT owned.

Even if we add the new ALLOCATED state, this device taken by failsafe is
also a valid port in new event callback in application, and is in 'NO 
OWNER'.
if event callback of application is called before one of failsafe PMD.
As a result, application still can operate this device directly.
Can we make sure that event callback of failsafe PMD is before one of 
application?
>
>
>
>
> .
lihuisong (C) Oct. 25, 2022, 3:26 a.m. UTC | #3 
在 2022/10/8 12:09, lihuisong (C) 写道:
>
> 在 2022/9/27 18:49, Thomas Monjalon 写道:
>> 15/09/2022 14:45, Huisong Li:
>>> The 'state' in struct rte_eth_dev may be used to update some 
>>> information
>>> when app receive these events. For example, when app receives a new 
>>> event,
>>> app may get the socket id of this port by calling 
>>> rte_eth_dev_socket_id to
>>> setup the attached port. The 'state' is used in rte_eth_dev_socket_id.
>>>
>>> If the state isn't modified to RTE_ETH_DEV_ATTACHED before pushing 
>>> the new
>>> event, app will get the socket id failed. So this patch moves 
>>> pushing event
>>> operation after the state updated.
>>>
>>> Fixes: 99a2dd955fba ("lib: remove librte_ prefix from directory names")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>> ---
>>>   lib/ethdev/ethdev_driver.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/lib/ethdev/ethdev_driver.c b/lib/ethdev/ethdev_driver.c
>>> index a285f213f0..a6616f072b 100644
>>> --- a/lib/ethdev/ethdev_driver.c
>>> +++ b/lib/ethdev/ethdev_driver.c
>>> @@ -206,9 +206,9 @@ rte_eth_dev_probing_finish(struct rte_eth_dev *dev)
>>>       if (rte_eal_process_type() == RTE_PROC_SECONDARY)
>>>           eth_dev_fp_ops_setup(rte_eth_fp_ops + dev->data->port_id, 
>>> dev);
>>>   -    rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_NEW, NULL);
>>>         dev->state = RTE_ETH_DEV_ATTACHED;
>>> +    rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_NEW, NULL);
>>>   }
>> As explained in the first patch, I don't think it is a good solution.
>> We should not allow the port to be used until the end of probing.
>> When RTE_ETH_EVENT_NEW is sent, the device is allocated but
>> not ready for use. If an entity like failsafe decides to take ownership
>> of the port, then the application should not consider it at all.
>> For these reasons, we should limit which operations can be done
>> during RTE_ETH_EVENT_NEW processing.
>> That's why I've proposed creating a new state RTE_ETH_DEV_ALLOCATED,
>> not sure why you didn't follow this advice.
> I want to put this problem to this case this patchset mentions, so as to
> have a better discussion. From the first patch, I know what you mean.
> But I still have some confusion:
> When a device taken by failsafe PMD push new event, all event callback
> will be called under this device. As you suggested, the device is a valid
> port if its state is ALLOCATED or ATTACHED. Now, the macro 
> RTE_ETH_FOREACH_DEV
> uses the condition that device is valid(state is ALLOCATED or ATTACHED)
> and NOT owned.
>
> Even if we add the new ALLOCATED state, this device taken by failsafe is
> also a valid port in new event callback in application, and is in 'NO 
> OWNER'.
> if event callback of application is called before one of failsafe PMD.
> As a result, application still can operate this device directly.
> Can we make sure that event callback of failsafe PMD is before one of 
> application?
Hi Thomas,
Can you look at my confusion?
>>
>>
>>
>>
>> .
> .
diff mbox series

Patch

diff --git a/lib/ethdev/ethdev_driver.c b/lib/ethdev/ethdev_driver.c
index a285f213f0..a6616f072b 100644
--- a/lib/ethdev/ethdev_driver.c
+++ b/lib/ethdev/ethdev_driver.c
@@ -206,9 +206,9 @@  rte_eth_dev_probing_finish(struct rte_eth_dev *dev)
 	if (rte_eal_process_type() == RTE_PROC_SECONDARY)
 		eth_dev_fp_ops_setup(rte_eth_fp_ops + dev->data->port_id, dev);
 
-	rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_NEW, NULL);
 
 	dev->state = RTE_ETH_DEV_ATTACHED;
+	rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_NEW, NULL);
 }
 
 int