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

[Thomas Monjalon <thomas@monjalon.net>]

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).

> 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.

> 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;      

> 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.