Re: [PATCH V4 1/5] drivers/bus: restore driver assignment at front of probing

[Ferruh Yigit <ferruh.yigit@amd.com>]

On 2/28/2023 2:21 AM, lihuisong (C) wrote:
> 
> 在 2023/2/16 0:09, Ferruh Yigit 写道:
>> On 1/12/2023 2:44 AM, lihuisong (C) wrote:
>>> 在 2023/1/11 20:51, Ferruh Yigit 写道:
>>>> On 12/6/2022 9:26 AM, Huisong Li wrote:
>>>>> The driver assignment was moved back at the end of the device probing
>>>>> because there is no something to use rte_driver during the phase of
>>>>> probing. See commit 391797f04208 ("drivers/bus: move driver assignment
>>>>> to end of probing")
>>>>>
>>>>> However, it is necessary for probing callback to reference rte_driver
>>>>> before probing. For example, probing callback may call some APIs which
>>>>> access the rte_pci_driver::driver by the device::driver pointer to get
>>>>> driver information. In this case, a segment fault will occur in probing
>>>>> callback if there is not this assignment.
>>>>>
>>>> Probing callback gets driver as parameter, so callback function can
>>>> access it via 'drv->driver', is there a specific usecase that
>>>> 'dev->device->driver' needs to be accessed explicitly?
>>>>
>>>> I assume this is related to coming patches that setting up device in
>>>> testpmd event callback, but can you please clarify exact need.
>>> For example, rte_eth_dev_info_get is called in this event callback to get
>>> driver name.
>>>
> Hi Ferruh,
> 
> Sorry for the delay. I missed this email.
> Thanks for your review.
>> Why 'rte_eth_dev_info_get()' is called in the event called at first place?
> There is no limitation on rte_eth_dev_info_get() in event callback.
> The upper layer is entirely possible to use.
> After all, it is more convenient for app to use this pointer to get
> driver information in a probing callback.
>

I think there is a logical limit in calling 'rte_eth_dev_info_get()' in
NEW event callback, we may document this if it is missing.

The NEW event callback is to let application do proper tasks before port
addition finalized in the ethdev layer, but you are trying to call
'rte_eth_dev_info_get()' in the callback when it is not ready yet.


>> This set updates multiple things to extend 'RTE_ETH_EVENT_NEW' event
>> callback function support, like:
>>
>> - Assign device driver *before* probing completed, so that even callback
>> can run 'rte_eth_dev_info_get()'
>>
>> - Add a new ethdev state so that port can be recognized as valid port in
>> the even callback.
> Yes
>> - Stop forwarding implicitly in even callback in case event callback run
>> while forwarding is on.
> But this is a modification for tesptmd to make it work well.
>> All looks to me hack/complexity to make a specific case work, which is
>> make secondary *testmp* application work with attached/detached device.
> It is not just a testpmd application case, but, I think, still exists in
> actual case.
> Because eal lib supports hotplugging device on primary and secondary
> process and the communication each other when attach or detach device.
> The reason why no one has ever put forward this question, I think it may
> be attributed to the fact that the scene is not or rarely tested.
>> And finally patch 4/5 adds port setup to testpmd event callback for this.
>>
>>
>> I understand the intention, but I disagree with bus and ethdev level
>> changes for this.
> I'm just raising this issue, and we can discuss how to deal with it
> together.😁
>>
>> Event callback may not be only way to share port attach/detach
>> information between primary and secondary, there is a MP socket and
>> 'rte_mp_handle' thread to handle communication between primary and
>> secondary process, this should be able to use carrying device
>> information, as far as I remember this is why it is introduced at first
>> place.
>>
>> Did you consider using MP socket for your use case?
> Actually, the probing event callback called in patch 4/5 is the result
> of the MP socket communication (please see hogplug_mp.c).
>>
>> Following is a sample usage:
>>
>> Primary:
>> started as:
>> sudo ./build/app/dpdk-testpmd --no-pci --proc-type=auto -l 0-1
>> --log-level=*:debug -- -i --num-procs=2 --proc-id=0
>>
>> ``
>> testpmd> show port summary all
>> Number of available ports: 0
>> Port MAC Address       Name         Driver         Status   Link
>>
>> testpmd> port attach net_null0
>> Attaching a new port...
>> dpaa: rte_dpaa_bus_parse(): Parse device name (net_null0 )
>> fslmc: rte_fslmc_parse(): Parsing dev=(net_null0 )
>> fslmc: rte_fslmc_parse(): Unknown or unsupported device (net_null0 )
>> vdev_probe_all_drivers(): Search driver to probe device net_null0
>> rte_pmd_null_probe(): Initializing pmd_null for net_null0
>> rte_pmd_null_probe(): Configure pmd_null: packet size is 64, packet copy
>> is disabled
>> eth_dev_null_create(): Creating null ethdev on numa socket 0
>> EAL: request: eal_dev_mp_request
>> EAL: msg: bus_vdev_mp
>> vdev_action(): send vdev, net_null0
>> EAL: sendmsg: bus_vdev_mp
>> EAL: reply: bus_vdev_mp
>> EAL: msg: eal_dev_mp_request
>> Port 0 is attached. Now total ports is 1
>> Done
>>
>> testpmd> show port summary all
>> Number of available ports: 1
>> Port MAC Address       Name         Driver         Status   Link
>> 0    DE:E5:79:00:A9:68 net_null0    net_null       down     10 Gbps
>>
>> testpmd> port detach 0
>> Port was not closed
>> Removing a device...
>> EAL: request: eal_dev_mp_request
>> EAL: msg: eal_dev_mp_request
>> eth_dev_close(): Closing null ethdev on NUMA socket 0
>> Port 0 is closed
>> Device is detached
>> Now total ports is 0
> Please note the log *"Now total ports is 0"*,
> which indicates the port number is updated if we detached device in this
> process.
>

Yes, as expected.


>> Done
>>
>> testpmd> show port summary all
>> Number of available ports: 0
>> Port MAC Address       Name         Driver         Status   Link
>> testpmd>
>>
>> ``
>>
>> Secondary:
>> started as:
>> sudo ./build/app/dpdk-testpmd --no-pci --proc-type=auto -l 2-3
>> --log-level=*:debug -- -i --num-procs=2 --proc-id=1
>>
>> ``
>> testpmd> show port summary all
>> Number of available ports: 0
>> Port MAC Address       Name         Driver         Status   Link
>>
>> testpmd> EAL: msg: eal_dev_mp_request
>> dpaa: rte_dpaa_bus_parse(): Parse device name (net_null0 )
>> fslmc: rte_fslmc_parse(): Parsing dev=(net_null0 )
>> fslmc: rte_fslmc_parse(): Unknown or unsupported device (net_null0 )
>> EAL: request: bus_vdev_mp
>> EAL: msg: bus_vdev_mp
>> vdev_action(): receive vdev, net_null0
>> EAL: msg: bus_vdev_mp
>> vdev_scan(): Received 1 vdevs
>> vdev_probe_all_drivers(): Search driver to probe device net_null0
>> rte_pmd_null_probe(): Initializing pmd_null for net_null0
>> EAL: reply: eal_dev_mp_request
>>
>> testpmd> show port summary all
>> Number of available ports: 1
>> Port MAC Address       Name         Driver         Status   Link
>> 0    DE:E5:79:00:A9:68 net_null0    net_null       down     10 Gbps
>>
>> testpmd> EAL: msg: eal_dev_mp_request
>> dpaa: rte_dpaa_bus_parse(): Parse device name (net_null0 )
>> fslmc: rte_fslmc_parse(): Parsing dev=(net_null0 )
>> fslmc: rte_fslmc_parse(): Unknown or unsupported device (net_null0 )
>> eth_dev_close(): Closing null ethdev on NUMA socket 4294967295
>> Port 0 is closed
>> EAL: reply: eal_dev_mp_request
> But the port number in this process does not be updated after finishing
> the destroy event.
> That's the problem.
>

Please see following command output,
it says "Number of available ports: 0",
so that shows port number is updated after destroy event, isn't it?


>> testpmd> show port summary all
>> Number of available ports: 0
>> Port MAC Address       Name         Driver         Status   Link
>> testpmd>
>> ``
>>
>>
>> .