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

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

在 2025/1/14 19:13, Thomas Monjalon 写道:
> 14/01/2025 02:50, lihuisong (C):
>> 在 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.
> We don't want an application to configure a port before probing is finished
> (like still in the event processing).
Ok
>
>> 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
>> ---
> I don't agree with configuring a port which is not fully probed.
Got it.
>
>> 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?
> Right, I think such application is abusing the new event.
>
> testpmd set a flag when receiving an event, it should not do more:
>
>      case RTE_ETH_EVENT_NEW:
>          ports[port_id].need_setup = 1;
>          ports[port_id].port_status = RTE_PORT_HANDLING;
>          break;
ok I know what you mean.
>> 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?
> As explained above, we should not do allow much API from RTE_ETH_EVENT_NEW.
> rte_eth_dev_socket_id() is reasonnable.
> Functions rte_eth_dev_owner_*() are fine.
> Others functions should be called only after probing.
All right, will fix it in new patch set.
And I'll also add these comments like above you said for RTE_ETH_EVENT_NEW.
>
>
>
> .