From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id D62E841D94; Tue, 28 Feb 2023 03:22:15 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 76CBF40DD8; Tue, 28 Feb 2023 03:22:15 +0100 (CET) Received: from szxga02-in.huawei.com (szxga02-in.huawei.com [45.249.212.188]) by mails.dpdk.org (Postfix) with ESMTP id 1FB0C40141 for ; Tue, 28 Feb 2023 03:22:13 +0100 (CET) Received: from kwepemm600004.china.huawei.com (unknown [172.30.72.57]) by szxga02-in.huawei.com (SkyGuard) with ESMTP id 4PQh0T352szKqHW; Tue, 28 Feb 2023 10:20:09 +0800 (CST) Received: from [10.67.103.231] (10.67.103.231) by kwepemm600004.china.huawei.com (7.193.23.242) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.21; Tue, 28 Feb 2023 10:21:34 +0800 Content-Type: multipart/alternative; boundary="------------4g7m6LBOqit9N06UseaDh0pc" Message-ID: <80f82803-0ea0-adf6-4dd6-ccafc6643fbd@huawei.com> Date: Tue, 28 Feb 2023 10:21:33 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 Subject: Re: [PATCH V4 1/5] drivers/bus: restore driver assignment at front of probing To: Ferruh Yigit , , CC: , , , References: <20220825024425.10534-1-lihuisong@huawei.com> <20221206092649.8287-1-lihuisong@huawei.com> <20221206092649.8287-2-lihuisong@huawei.com> <79192ffa-eb80-e56c-5c64-4a22374c7a1a@amd.com> <461a4536-fbbd-3848-af8d-2aa1ebaf8ccb@huawei.com> <5d06f478-6142-651b-d54b-0a3c75e9e7a0@amd.com> From: "lihuisong (C)" In-Reply-To: <5d06f478-6142-651b-d54b-0a3c75e9e7a0@amd.com> X-Originating-IP: [10.67.103.231] X-ClientProxiedBy: dggems702-chm.china.huawei.com (10.3.19.179) To kwepemm600004.china.huawei.com (7.193.23.242) X-CFilter-Loop: Reflected X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org --------------4g7m6LBOqit9N06UseaDh0pc Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit 在 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. > > 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. > 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. > testpmd> show port summary all > Number of available ports: 0 > Port MAC Address Name Driver Status Link > testpmd> > `` > > > . --------------4g7m6LBOqit9N06UseaDh0pc Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: 8bit


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

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.
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.
testpmd> show port summary all
Number of available ports: 0
Port MAC Address       Name         Driver         Status   Link
testpmd>
``


.
--------------4g7m6LBOqit9N06UseaDh0pc--