Re: [PATCH v2 2/2] crypto/ipsec_mb: fix QP release in secondary

[Moses Young <mosesyyoung@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 6643 bytes --]

On 5/19/2025 6:46 PM, Ji, Kai wrote:

> Hi Yangming,
>
> How did you configure the queue pairs differently between the primary 
> and secondary processes?
>
> The application must call rte_cryptodev_queue_pair_setup() with a 
> unique qp_id for each process. This implies that each process should 
> receive distinct queue pair configurations at runtime. From what I can 
> tell, it’s likely that the secondary process is still using the same 
> queue pair as the primary process.
>
> Regards
>
> Kai
>
>
>
>
> ------------------------------------------------------------------------
> *From:* Moses Young <mosesyyoung@gmail.com>
> *Sent:* Monday, May 12, 2025 11:10
> *To:* Ji, Kai <kai.ji@intel.com>; Yang Ming 
> <ming.1.yang@nokia-sbell.com>; dev@dpdk.org <dev@dpdk.org>
> *Subject:* Re: [PATCH v2 2/2] crypto/ipsec_mb: fix QP release in 
> secondary
>
> On 5/7/2025 11:25 PM, Ji, Kai wrote:
>
>     Hi Yangming,
>
>     PID check is implemented here:
>     https://github.com/DPDK/dpdk/blob/75f179ebe347b6098cf3af26d3d3b7168fe3fe24/drivers/crypto/ipsec_mb/ipsec_mb_ops.c#L376
>     <https://github.com/DPDK/dpdk/blob/75f179ebe347b6098cf3af26d3d3b7168fe3fe24/drivers/crypto/ipsec_mb/ipsec_mb_ops.c#L376>
>
>     Can you share the steps to re-produce the error ?
>
>     Regards
>
>     Kai
>
>     ------------------------------------------------------------------------
>     *From:* Yang Ming
>     *Sent:* Thursday, April 24, 2025 15:26
>     *To:* dev@dpdk.org <mailto:dev@dpdk.org>
>     *Subject:* Re: [PATCH v2 2/2] crypto/ipsec_mb: fix QP release in
>     secondary
>
>     Hi,
>
>     On 2025/4/7 13:25, Yang Ming wrote:
>     > From: myang <ming.1.yang@nokia-sbell.com>
>     <mailto:ming.1.yang@nokia-sbell.com>
>     >
>     > When a secondary process tries to release a queue pair (QP) that
>     > does not belong to it, error logs occur:
>     > CRYPTODEV: ipsec_mb_ipc_request() line 373: Unable to release
>     > qp_id=0
>     > EAL: Message data is too long
>     > EAL: Fail to handle message: ipsec_mb_mp_msg
>     > EAL: Fail to recv reply for request /tmp/dpdk/l2hi/mp_socket:
>     > ipsec_mb_mp_msg
>     >
>     > This patch ensures that a secondary process only frees a QP if
>     > it actually owns it, preventing conflicts and resolving the
>     > issue.
>     >
>     > Signed-off-by: myang <ming.1.yang@nokia-sbell.com>
>     <mailto:ming.1.yang@nokia-sbell.com>
>     > ---
>     >   drivers/crypto/ipsec_mb/ipsec_mb_ops.c | 7 +++++--
>     >   1 file changed, 5 insertions(+), 2 deletions(-)
>     >
>     > diff --git a/drivers/crypto/ipsec_mb/ipsec_mb_ops.c
>     b/drivers/crypto/ipsec_mb/ipsec_mb_ops.c
>     > index 910efb1a97..50ee140ccd 100644
>     > --- a/drivers/crypto/ipsec_mb/ipsec_mb_ops.c
>     > +++ b/drivers/crypto/ipsec_mb/ipsec_mb_ops.c
>     > @@ -138,6 +138,7 @@ int
>     >   ipsec_mb_qp_release(struct rte_cryptodev *dev, uint16_t qp_id)
>     >   {
>     >        struct ipsec_mb_qp *qp = dev->data->queue_pairs[qp_id];
>     > +     uint16_t process_id = (uint16_t)getpid();
>     >
>     >        if (!qp)
>     >                return 0;
>     > @@ -152,8 +153,10 @@ ipsec_mb_qp_release(struct rte_cryptodev
>     *dev, uint16_t qp_id)
>     >                rte_free(qp);
>     >                dev->data->queue_pairs[qp_id] = NULL;
>     >        } else { /* secondary process */
>     > -             return ipsec_mb_secondary_qp_op(dev->data->dev_id,
>     qp_id,
>     > -                                     NULL, 0,
>     RTE_IPSEC_MB_MP_REQ_QP_FREE);
>     > +             if (qp->qp_used_by_pid == process_id)
>     > +                     return
>     ipsec_mb_secondary_qp_op(dev->data->dev_id,
>     > +                                             qp_id, NULL, 0,
>     > + RTE_IPSEC_MB_MP_REQ_QP_FREE);
>     >        }
>     >        return 0;
>     >   }
>
>     Hi Experts,
>
>     Is there any chance to review and accept this patch?
>
>     Brs,
>
>     Yang Ming
>
> Hi,
>
> David. Thanks for your feedback. I will Cc maintainers in next version.
>
> Kai, Thanks for your feedback. Here's our test steps after applying 
> the previous patch called "eal: prevent socket closure before MP sync" 
> in this serious:
> 1. Start the primary process: Run the DPDK primary process with the 
> IPsec MB crypto device.
> 2. Launch the secondary process: Start a DPDK secondary process using 
> the same device parameters.
> 3. Exit the secondary normally.
>
> On secondary exit, error logs show below as my commit log's description:
> CRYPTODEV: ipsec_mb_ipc_request() line 373: Unable to release qp_id=0
> EAL: Message data is too long
> EAL: Fail to handle message: ipsec_mb_mp_msg
> EAL: Fail to recv reply for request /tmp/dpdk/l2hi/mp_socket: 
> ipsec_mb_mp_msg
>
> This message corresponds exactly to the PID check in the code you 
> referenced:
> if (qp->qp_used_by_pid != req_param->process_id) {
>     CDEV_LOG_ERR("Unable to release qp_id=%d", qp_id);
>     goto out;
> }
>
> In our view, this log indicates an abnormal condition: a secondary 
> process should be able to release its own QPs without triggering an 
> error during normal shutdown.
>
> Thanks for your help.
>
> Best,
> Yang Ming


Hi Kai,

The queue pairs is shared between the primary and secondary processes. 
Even if each process calls rte_cryptodev_queue_pair_setup() with a 
unique qp_id, the primary and secondary still end up using the same 
shared queue pair structure, because cryptodev->data is shared between them.

 From the code path, cryptodev->data is allocated in the primary via 
rte_cryptodev_data_alloc() (inside 
ipsec_mb_create-->rte_cryptodev_pmd_create-->rte_cryptodev_pmd_allocate-->rte_cryptodev_data_alloc). 
This memory is placed in a shared memzone (rte_cryptodev_data_%u), so 
both primary and secondary processes reference the same cryptodev->data, 
including nb_queue_pairs and queue_pairs[].
As a result, when the secondary process exits, ipsec_mb_remove() is 
called (inside 
rte_eal_cleanup-->eal_bus_cleanup-->vdev_cleanup-->rte_vdev_driver-->ipsec_mb_remove-->ipsec_mb_qp_release-->ipsec_mb_secondary_qp_op) 
and it loops through all queue pairs using:
for (qp_id = 0; qp_id < cryptodev->data->nb_queue_pairs; qp_id++)
     ipsec_mb_qp_release(cryptodev, qp_id);

This causes the secondary to attempt releasing queue pairs it doesn't 
own, triggering the error logs mentioned in previous mail.

Best regards,
Yang Ming

[-- Attachment #2: Type: text/html, Size: 13408 bytes --]