Re: [PATCH] ethdev: fix push new event

["lihuisong (C)" <lihuisong@huawei.com>]

在 2022/9/27 18:29, Thomas Monjalon 写道:
> 11/06/2022 10:59, lihuisong (C):
>> 在 2022/6/7 14:44, Thomas Monjalon 写道:
>>> 07/06/2022 03:23, lihuisong (C):
>>>> 在 2022/6/3 15:42, Thomas Monjalon 写道:
>>>>> 02/06/2022 13:24, lihuisong (C):
>>>>>> 在 2022/5/30 19:10, Ferruh Yigit 写道:
>>>>>>> On 5/30/2022 9:28 AM, Thomas Monjalon wrote:
>>>>>>>> [CAUTION: External Email]
>>>>>>>>
>>>>>>>> 28/05/2022 10:53, lihuisong (C):
>>>>>>>>> 在 2022/5/23 22:36, Thomas Monjalon 写道:
>>>>>>>>>> 23/05/2022 11:51, David Marchand:
>>>>>>>>>>> On Sat, May 21, 2022 at 8:57 AM Min Hu
>>>>>>>>>>> (Connor)<humin29@huawei.com>  wrote:
>>>>>>>>>>>> From: Huisong Li<lihuisong@huawei.com>
>>>>>>>>>>>>
>>>>>>>>>>>> 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")
>>>>>>>>>>> A patch moving code is unlikely to be at fault.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Looking at the patch which moved those notifications in this point of
>>>>>>>>>>> the code, the state update was pushed after the notification on
>>>>>>>>>>> purpose.
>>>>>>>>>>> See be8cd210379a ("ethdev: fix port probing notification")
>>>>>>>>>>>
>>>>>>>>>>>          ethdev: fix port probing notification
>>>>>>>>>>>
>>>>>>>>>>>          The new device was notified as soon as it was allocated.
>>>>>>>>>>>          It leads to use a device which is not yet initialized.
>>>>>>>>>>>
>>>>>>>>>>>          The notification must be published after the initialization
>>>>>>>>>>> is done
>>>>>>>>>>>          by the PMD, but before the state is changed, in order to let
>>>>>>>>>>>          notified entities taking ownership before general availability.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Do we need an intermediate state during probing?
>>>>>>>>>> Possibly. Currently we have only 3 states:
>>>>>>>>>>         RTE_ETH_DEV_UNUSED
>>>>>>>>>>         RTE_ETH_DEV_ATTACHED
>>>>>>>>>>         RTE_ETH_DEV_REMOVED
>>>>>>>>>>
>>>>>>>>>> We may add RTE_ETH_DEV_ALLOCATED just before calling
>>>>>>>>>>         rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_NEW, NULL);
>>>>>>>>>> Then we would need to check against RTE_ETH_DEV_ALLOCATED
>>>>>>>>>> in some ethdev functions.
>>>>>>>>>>
>>>>>>>>> Hi, Thomas,
>>>>>>>>>
>>>>>>>>> Do you mean that we need to modify some funcions like following?
>>>>>>>>>
>>>>>>>>> int rte_eth_dev_is_valid_port(uint16_t port_id)
>>>>>>>>> {
>>>>>>>>>          if (port_id >= RTE_MAX_ETHPORTS ||
>>>>>>>>>              (rte_eth_devices[port_id].state != *RTE_ETH_DEV_ALLOCATED*))
>>>>>>>>>              return 0;
>>>>>>> Won't this mark ATTACHED devices as invalid?
>>>>>> Yes, You are right.
>>>>>>
>>>>>>> If the state flow will be as UNUSED -> ALLOCATED -> ATTACHED, above
>>>>>>> check should be against 'ATTACHED' I think.
>>>>> It should validate both ALLOCATED and ATTACHED.
>>>> Actually, we can only pick one, because it is an enumeration.
>>> You can check it is either one state or the other.
>> uint16_t
>> rte_eth_find_next(uint16_t port_id)
>> {
>>       while (port_id < RTE_MAX_ETHPORTS &&
>>              !(rte_eth_devices[port_id].state == RTE_ETH_DEV_ALLOCATED ||
>>                rte_eth_devices[port_id].state == RTE_ETH_DEV_ATTACHED))
>>           port_id++;
>>
>>       if (port_id >= RTE_MAX_ETHPORTS)
>>           return RTE_MAX_ETHPORTS;
>>
>>       return port_id;
>> }
>> like this, right? If so, adding 'ALLOCATED' and setting to 'ALLOCATED'
>> is the same with
>> setting to 'ATTACHED' before sending new event.
>> They both meet the requirements mentioned in this patch that the device
>> is a valid port
>> when applications receive a new event.
> Yes, when receiving the event, the port would valid
> in state ALLOCATED.
> Then we can set as ATTACHED when definitely initialized,
> after the notifications.
>
>> However, if device is taken by failsafe PMD as sub-device, the
>> processing above
>> still doesn't satisfy the purpose of failsafe PMD when this sub-device
>> push new event.
> I don't understand why you think failsafe is not satisfied.
>> I don't know if I'm missing something. Can you explain it, Ferruh and
>> Thomas?
> Please explain what you think is failing with failsafe.
Please look the reply in new patchset.
>
>
>
> .