Re: [PATCH v1 2/2] ethdev: fix skip valid port in probing callback

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

在 2025/1/13 21:14, Thomas Monjalon 写道:
> 13/01/2025 13:47, lihuisong (C):
>> 在 2025/1/13 20:30, Thomas Monjalon 写道:
>>> 13/01/2025 13:05, lihuisong (C):
>>>> 在 2025/1/13 19:23, lihuisong (C) 写道:
>>>>> 在 2025/1/13 18:57, Thomas Monjalon 写道:
>>>>>> 13/01/2025 10:35, lihuisong (C):
>>>>>>> 在 2025/1/13 16:16, Thomas Monjalon 写道:
>>>>>>>> 13/01/2025 03:55, Huisong Li:
>>>>>>>>> The event callback in application may use the macro
>>>>>>>>> RTE_ETH_FOREACH_DEV to
>>>>>>>>> iterate over all enabled ports to do something(like, verifying the
>>>>>>>>> port id
>>>>>>>>> validity) when receive a probing event. If the ethdev state of a
>>>>>>>>> port is
>>>>>>>>> not RTE_ETH_DEV_UNUSED, this port will be considered as a valid port.
>>>>>>>>>
>>>>>>>>> However, this state is set to RTE_ETH_DEV_ATTACHED after pushing
>>>>>>>>> probing
>>>>>>>>> event. It means that probing callback will skip this port. But this
>>>>>>>>> assignment can not move to front of probing notification. See
>>>>>>>>> commit be8cd210379a ("ethdev: fix port probing notification")
>>>>>>>>>
>>>>>>>>> So this patch has to add a new state, RTE_ETH_DEV_ALLOCATED. Set
>>>>>>>>> the ethdev
>>>>>>>>> state to RTE_ETH_DEV_ALLOCATED before pushing probing event and
>>>>>>>>> set it to
>>>>>>>>> RTE_ETH_DEV_ATTACHED after definitely probed. And this port is
>>>>>>>>> valid if its
>>>>>>>>> device state is 'ALLOCATED' or 'ATTACHED'.
>>>>>>>> If you do that, changing the definition of eth_dev_find_free_port()
>>>>>>>> you allow the application using a port before probing is finished.
>>>>>>> Yes, it's not reasonable.
>>>>>>>
>>>>>>> Thinking your comment twice, I feel that the root cause of this
>>>>>>> issue is
>>>>>>> application want to check if the port id is valid.
>>>>>>> However, application just receive the new event from the device and the
>>>>>>> port id of this device must be valid when report new event.
>>>>>>> So application can think the received new event is valid and don't need
>>>>>>> to check, right?
>>>>>> Yes
>>>>>> Do you think it should be highlighted in the API doc?
>>>>> Security detection is common and always good for application.
>>>>> So I think it's better to highlight that in doc.
>>>>>
>>>> Now I remember why I have to put this patch into the patchset [1] that
>>>> testpmd support multiple process attach and detach port.
>>>> Becase patch 4/5 in this series depands on this patch.
>>>> The setup_attached_port() have to move to eth_event_callback() in
>>>> testpmd to update something.
>>>> And the setup_attached_port() would indirectyly check if this port is
>>>> valid by rte_eth_dev_is_valid_port().
>>>> Their caller stack is as follows:
>>>> eth_event_callback
>>>>        -->setup_attached_port
>>>>            -->rte_eth_dev_socket_id
>>>>                -->rte_eth_dev_is_valid_port
>>>>
>>>>    From the testpmd's modification, that is to say, it is possible for
>>>> appllication to call some APIs like rte_eth_dev_socket_id() and
>>>> indirectyly check if this port id is valid in event new callback.
>>>> So should we add this patch? I think there are many like these API in
>>>> ethdev layer. I'm confused a bit now.
>>> Yes rte_eth_dev_is_valid_port() is used in many API functions,
>>> so that's a valid concern.
>>> I would say we should not call much of these functions in the "new port"
>>> event callback.
>>> But the case of rte_eth_dev_socket_id() is concerning.
>>>
>>> I suggest to update rte_eth_dev_socket_id() to make it work with
>>> a newly allocated port.
>>> I suppose we can use the function eth_dev_is_allocated().
>> What you mean is doing it like the following code?
>> -->
>>
>> --- a/lib/ethdev/rte_ethdev.c
>> +++ b/lib/ethdev/rte_ethdev.c
>> @@ -635,8 +635,10 @@ int
>>    rte_eth_dev_socket_id(uint16_t port_id)
>>    {
>>           int socket_id = SOCKET_ID_ANY;
>> +       struct rte_eth_dev *ethdev;
>>
>> -       if (!rte_eth_dev_is_valid_port(port_id)) {
>> +       ethdev = &rte_eth_devices[port_id];
>> +       if (!eth_dev_is_allocated(ethdev)) {
>>                   rte_errno = EINVAL;
>>           } else {
>>                   socket_id = rte_eth_devices[port_id].data->numa_node;
>
> Yes. Would it work?
I think it can work for this API.

 From the disscussion for this patch, we've come to an aggreement that 
application can think port is valid in new event.
Now that the port id is valid, the new event callback of application may 
call other API, for example, rte_eth_dev_info_get().
(Apllication may call rte_eth_dev_info_get to get someting in new event 
callback)
Note: patch 4/5 modified in the series[1] also used this API.
-->
eth_event_callback
     -->setup_attached_port
         -->reconfig
             -->init_config_port_offloads
                 -->eth_dev_info_get_print_err
---

There is RTE_ETH_VALID_PORTID_OR_ERR_RET to check port_id is valid in 
rte_eth_dev_info_get.
Application also happen to this issue like rte_eth_dev_socket_id, right?
This macro is also widely used in ethdev layer. We probability need to 
filter out all these interfaces which can be used in new event callback.
And then handle the check for port_id in these interfaces like 
rte_eth_dev_socket_id.
What do you think?  Are there any other similar interfaces in ethdev layer?


>
>
>
>
> .