diff mbox series

[v2] cryptodev: not close device when secondary exit

Message ID 20250103030957.1117-1-ming.1.yang@nokia-sbell.com (mailing list archive)
State Accepted
Delegated to: akhil goyal
Headers
Series [v2] cryptodev: not close device when secondary exit |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/Intel-compilation success Compilation OK
ci/github-robot: build success github build: passed
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-unit-arm64-testing pending Testing pending
ci/iol-marvell-Functional success Functional Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-abi-testing pending Testing pending
ci/iol-unit-amd64-testing pending Testing pending
ci/iol-compile-arm64-testing success Testing PASS
ci/iol-sample-apps-testing success Testing PASS
ci/intel-Testing success Testing PASS
ci/intel-Functional success Functional PASS

Commit Message

Yang Ming Jan. 3, 2025, 3:09 a.m. UTC 
The secordary process should not close the crypto device when
it exits because the primary process still manage the device.
There is no reason with occurring error log below when
secordary process exits without any operation on the crypto
device while primary process starts the device.

Case situation:
eal_bus_cleanup has been added in rte_eal_cleanup. But for
secondary process, eal_bus_cleanup will trigger vdev_cleanup
which trigger rte_vdev_driver to remove. Then crypto devices
will execute ipsec_mb_remove to rte_cryptodev_pmd_destroy.
Finially, rte_cryptodev_close will be called by secordary
process exit.

Error logs occur as below when the secordary process exit:
CRYPTODEV: rte_cryptodev_close() line 1453: Device 0 must be
stopped before closing

Function call trace: rte_eal_cleanup->eal_bus_cleanup->
vdev_cleanup->rte_vdev_driver_remove->ipsec_mb_remove->
rte_cryptodev_pmd_destroy->rte_cryptodev_pmd_release_device->
rte_cryptodev_close

Signed-off-by: Yang Ming <ming.1.yang@nokia-sbell.com>
---
 lib/cryptodev/rte_cryptodev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Akhil Goyal Jan. 8, 2025, 12:21 p.m. UTC | #1 
> The secordary process should not close the crypto device when
> it exits because the primary process still manage the device.
> There is no reason with occurring error log below when
> secordary process exits without any operation on the crypto
> device while primary process starts the device.
> 
> Case situation:
> eal_bus_cleanup has been added in rte_eal_cleanup. But for
> secondary process, eal_bus_cleanup will trigger vdev_cleanup
> which trigger rte_vdev_driver to remove. Then crypto devices
> will execute ipsec_mb_remove to rte_cryptodev_pmd_destroy.
> Finially, rte_cryptodev_close will be called by secordary
> process exit.
> 
> Error logs occur as below when the secordary process exit:
> CRYPTODEV: rte_cryptodev_close() line 1453: Device 0 must be
> stopped before closing
> 
> Function call trace: rte_eal_cleanup->eal_bus_cleanup->
> vdev_cleanup->rte_vdev_driver_remove->ipsec_mb_remove->
> rte_cryptodev_pmd_destroy->rte_cryptodev_pmd_release_device->
> rte_cryptodev_close
> 
> Signed-off-by: Yang Ming <ming.1.yang@nokia-sbell.com>
> ---
>  lib/cryptodev/rte_cryptodev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/cryptodev/rte_cryptodev.c b/lib/cryptodev/rte_cryptodev.c
> index 85a4b46ac9..ed1021f635 100644
> --- a/lib/cryptodev/rte_cryptodev.c
> +++ b/lib/cryptodev/rte_cryptodev.c
> @@ -1142,7 +1142,7 @@ rte_cryptodev_pmd_release_device(struct
> rte_cryptodev *cryptodev)
>  	cryptodev_fp_ops_reset(rte_crypto_fp_ops + dev_id);
> 
>  	/* Close device only if device operations have been set */
> -	if (cryptodev->dev_ops) {
> +	if (cryptodev->dev_ops && (rte_eal_process_type() ==
> RTE_PROC_PRIMARY)) {
>  		ret = rte_cryptodev_close(dev_id);
>  		if (ret < 0)
>  			return ret;
I believe dev_close is actually not required in pmd_release_device.
Dev_close need to be called from the application separately before it is released
which I think is already happening. 

Adding more people for review.
Yang Ming Feb. 20, 2025, 9:13 a.m. UTC | #2 
On 2025/1/8 20:21, Akhil Goyal wrote:
> Caution: This is an external email. Please be very careful when clicking links or opening attachments. See http://nok.it/nsb for additional information.
>
>> The secordary process should not close the crypto device when
>> it exits because the primary process still manage the device.
>> There is no reason with occurring error log below when
>> secordary process exits without any operation on the crypto
>> device while primary process starts the device.
>>
>> Case situation:
>> eal_bus_cleanup has been added in rte_eal_cleanup. But for
>> secondary process, eal_bus_cleanup will trigger vdev_cleanup
>> which trigger rte_vdev_driver to remove. Then crypto devices
>> will execute ipsec_mb_remove to rte_cryptodev_pmd_destroy.
>> Finially, rte_cryptodev_close will be called by secordary
>> process exit.
>>
>> Error logs occur as below when the secordary process exit:
>> CRYPTODEV: rte_cryptodev_close() line 1453: Device 0 must be
>> stopped before closing
>>
>> Function call trace: rte_eal_cleanup->eal_bus_cleanup->
>> vdev_cleanup->rte_vdev_driver_remove->ipsec_mb_remove->
>> rte_cryptodev_pmd_destroy->rte_cryptodev_pmd_release_device->
>> rte_cryptodev_close
>>
>> Signed-off-by: Yang Ming <ming.1.yang@nokia-sbell.com>
>> ---
>>   lib/cryptodev/rte_cryptodev.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/lib/cryptodev/rte_cryptodev.c b/lib/cryptodev/rte_cryptodev.c
>> index 85a4b46ac9..ed1021f635 100644
>> --- a/lib/cryptodev/rte_cryptodev.c
>> +++ b/lib/cryptodev/rte_cryptodev.c
>> @@ -1142,7 +1142,7 @@ rte_cryptodev_pmd_release_device(struct
>> rte_cryptodev *cryptodev)
>>   	cryptodev_fp_ops_reset(rte_crypto_fp_ops + dev_id);
>>
>>   	/* Close device only if device operations have been set */
>> -	if (cryptodev->dev_ops) {
>> +	if (cryptodev->dev_ops && (rte_eal_process_type() ==
>> RTE_PROC_PRIMARY)) {
>>   		ret = rte_cryptodev_close(dev_id);
>>   		if (ret < 0)
>>   			return ret;
> I believe dev_close is actually not required in pmd_release_device.
> Dev_close need to be called from the application separately before it is released
> which I think is already happening.
>
> Adding more people for review.
>
>
Hi Akhil, I suppose the change of removing `dev_close` in 
`pmd_release_device` requires much more effort and commit size because 
this change will impact a lot of examples and cases. As there is no more 
comments from other people, can we accept this patch as a little 
improvement for the second process's scenario and wait for the entire 
improvement from anyone who is interested in this entire work?
Akhil Goyal March 2, 2025, 8:24 p.m. UTC | #3 
> >> The secordary process should not close the crypto device when
> >> it exits because the primary process still manage the device.
> >> There is no reason with occurring error log below when
> >> secordary process exits without any operation on the crypto
> >> device while primary process starts the device.
> >>
> >> Case situation:
> >> eal_bus_cleanup has been added in rte_eal_cleanup. But for
> >> secondary process, eal_bus_cleanup will trigger vdev_cleanup
> >> which trigger rte_vdev_driver to remove. Then crypto devices
> >> will execute ipsec_mb_remove to rte_cryptodev_pmd_destroy.
> >> Finially, rte_cryptodev_close will be called by secordary
> >> process exit.
> >>
> >> Error logs occur as below when the secordary process exit:
> >> CRYPTODEV: rte_cryptodev_close() line 1453: Device 0 must be
> >> stopped before closing
> >>
> >> Function call trace: rte_eal_cleanup->eal_bus_cleanup->
> >> vdev_cleanup->rte_vdev_driver_remove->ipsec_mb_remove->
> >> rte_cryptodev_pmd_destroy->rte_cryptodev_pmd_release_device->
> >> rte_cryptodev_close
> >>
> >> Signed-off-by: Yang Ming <ming.1.yang@nokia-sbell.com>
> >> ---
> >>   lib/cryptodev/rte_cryptodev.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/lib/cryptodev/rte_cryptodev.c b/lib/cryptodev/rte_cryptodev.c
> >> index 85a4b46ac9..ed1021f635 100644
> >> --- a/lib/cryptodev/rte_cryptodev.c
> >> +++ b/lib/cryptodev/rte_cryptodev.c
> >> @@ -1142,7 +1142,7 @@ rte_cryptodev_pmd_release_device(struct
> >> rte_cryptodev *cryptodev)
> >>   	cryptodev_fp_ops_reset(rte_crypto_fp_ops + dev_id);
> >>
> >>   	/* Close device only if device operations have been set */
> >> -	if (cryptodev->dev_ops) {
> >> +	if (cryptodev->dev_ops && (rte_eal_process_type() ==
> >> RTE_PROC_PRIMARY)) {
> >>   		ret = rte_cryptodev_close(dev_id);
> >>   		if (ret < 0)
> >>   			return ret;
> > I believe dev_close is actually not required in pmd_release_device.
> > Dev_close need to be called from the application separately before it is released
> > which I think is already happening.
> >
> > Adding more people for review.
> >
> >
> Hi Akhil, I suppose the change of removing `dev_close` in
> `pmd_release_device` requires much more effort and commit size because
> this change will impact a lot of examples and cases. As there is no more
> comments from other people, can we accept this patch as a little
> improvement for the second process's scenario and wait for the entire
> improvement from anyone who is interested in this entire work?

Acked-by: Akhil Goyal <gakhil@marvell.com>

Applied to dpdk-next-crypto
diff mbox series

Patch

diff --git a/lib/cryptodev/rte_cryptodev.c b/lib/cryptodev/rte_cryptodev.c
index 85a4b46ac9..ed1021f635 100644
--- a/lib/cryptodev/rte_cryptodev.c
+++ b/lib/cryptodev/rte_cryptodev.c
@@ -1142,7 +1142,7 @@  rte_cryptodev_pmd_release_device(struct rte_cryptodev *cryptodev)
 	cryptodev_fp_ops_reset(rte_crypto_fp_ops + dev_id);
 
 	/* Close device only if device operations have been set */
-	if (cryptodev->dev_ops) {
+	if (cryptodev->dev_ops && (rte_eal_process_type() == RTE_PROC_PRIMARY)) {
 		ret = rte_cryptodev_close(dev_id);
 		if (ret < 0)
 			return ret;