bug in cryptodev enqueue/dequeue callbacks?

bug in cryptodev enqueue/dequeue callbacks?

[Konstantin Ananyev]

Hi everyone,

Looking at implementation of cryptodev callbacks
(it uses DPDK RCU), it seems like there is a bug here:

at init time we don't call rte_rcu_qsbr_thread_register().
As I understand without it rte_rcu_qsbr_check() wouldn't
work properly for that thread.

Probably need to add:
static int
cryptodev_cb_init(struct rte_cryptodev *dev)
{
 	....
	if (rte_rcu_qsbr_init(qsbr, max_threads)) {...}
+	rte_rcu_qsbr_thread_register(qsbr, 0);		

Unless I am missing something obvious here?
Konstantin

Re: bug in cryptodev enqueue/dequeue callbacks?

[Honnappa Nagarahalli]

> On Nov 14, 2024, at 8:41 AM, Konstantin Ananyev <konstantin.ananyev@huawei.com> wrote:
> 
> Hi everyone,
> 
> Looking at implementation of cryptodev callbacks
> (it uses DPDK RCU), it seems like there is a bug here:
> 
> at init time we don't call rte_rcu_qsbr_thread_register().
> As I understand without it rte_rcu_qsbr_check() wouldn't
> work properly for that thread.
Yes, this understanding is correct. However, the responsibility of calling the rte_rcu_qsbr_thread_register lies with the application. The roles and responsibilities are documented at [1]

[1] https://doc.dpdk.org/guides/prog_guide/rcu_lib.html#resource-reclamation-framework-for-dpdk

> 
> Probably need to add:
> static int
> cryptodev_cb_init(struct rte_cryptodev *dev)
> {
> ....
> if (rte_rcu_qsbr_init(qsbr, max_threads)) {...}
> + rte_rcu_qsbr_thread_register(qsbr, 0); 
> 
> Unless I am missing something obvious here?
> Konstantin
> 

RE: bug in cryptodev enqueue/dequeue callbacks?

[Konstantin Ananyev]

 
> > On Nov 14, 2024, at 8:41 AM, Konstantin Ananyev <konstantin.ananyev@huawei.com> wrote:
> >
> > Hi everyone,
> >
> > Looking at implementation of cryptodev callbacks
> > (it uses DPDK RCU), it seems like there is a bug here:
> >
> > at init time we don't call rte_rcu_qsbr_thread_register().
> > As I understand without it rte_rcu_qsbr_check() wouldn't
> > work properly for that thread.
> Yes, this understanding is correct. However, the responsibility of calling the rte_rcu_qsbr_thread_register lies with the application.
> The roles and responsibilities are documented at [1]
> 
> [1] https://doc.dpdk.org/guides/prog_guide/rcu_lib.html#resource-reclamation-framework-for-dpdk

I don't think it is an option here.
cryptodev callbacks adding/removing/invoking mechanism it totally opaque to the user.
So these RCU instances are internal for cryptodev devices and shouldn't be updated for the user.
In fact, it is sort of 'speacial-case' where we have rcu with just one thread_id(==0),
and doing online/offline only.
Here is a link to original discussion:
https://mails.dpdk.org/archives/dev/2020-September/183401.html
So I still think we just forgot to call 
rte_rcu_qsbr_thread_register(qsbr, 0);
here.
 
> >
> > Probably need to add:
> > static int
> > cryptodev_cb_init(struct rte_cryptodev *dev)
> > {
> > ....
> > if (rte_rcu_qsbr_init(qsbr, max_threads)) {...}
> > + rte_rcu_qsbr_thread_register(qsbr, 0);
> >
> > Unless I am missing something obvious here?
> > Konstantin
> >