Re: [dpdk-dev] [PATCH 3/8] cryptodev: add helper functions for new datapath interface

[Akhil Goyal <gakhil@marvell.com>]

Hi Fan,
> Hi Akhil,
> 
> > +__rte_internal
> > +static inline void *
> > +_rte_cryptodev_dequeue_prolog(uint8_t dev_id, uint8_t qp_id)
> > +{
> > +	struct rte_cryptodev *dev = &rte_cryptodevs[dev_id];
> > +
> > +	return dev->data->queue_pairs[qp_id];
> > +}
> 
> [Fan: the function name looks unclear to me - maybe
> rte_cryptodev_dequeue_prepare?

The naming convention is same as Konstantin did for ethdev and
Subsequently by Pavan in eventdev. I think it is better to align all
With similar naming.

> Also the function didn't do any check as the description suggested - the
> qp is later checked in _RTE_CRYPTO_DEQ_DEF, maybe remove the
> description?]
> 

Ok will update the description in next version

> > +_rte_cryptodev_dequeue_epilog(uint16_t dev_id, uint16_t qp_id,
> > +	struct rte_crypto_op **ops, uint16_t nb_ops)
> > +{
> > +#ifdef RTE_CRYPTO_CALLBACKS
> > +	struct rte_cryptodev *dev = &rte_cryptodevs[dev_id];
> > +
> > +	if (unlikely(dev->deq_cbs != NULL)) {
> > +		struct rte_cryptodev_cb_rcu *list;
> > +		struct rte_cryptodev_cb *cb;
> > +
> > +		/* __ATOMIC_RELEASE memory order was used when the
> > +		 * call back was inserted into the list.
> > +		 * Since there is a clear dependency between loading
> > +		 * cb and cb->fn/cb->next, __ATOMIC_ACQUIRE memory
> > order is
> > +		 * not required.
> > +		 */
> > +		list = &dev->deq_cbs[qp_id];
> > +		rte_rcu_qsbr_thread_online(list->qsbr, 0);
> > +		cb = __atomic_load_n(&list->next, __ATOMIC_RELAXED);
> > +
> > +		while (cb != NULL) {
> > +			nb_ops = cb->fn(dev_id, qp_id, ops, nb_ops,
> > +					cb->arg);
> > +			cb = cb->next;
> > +		};
> > +
> > +		rte_rcu_qsbr_thread_offline(list->qsbr, 0);
> > +	}
> > +#endif
> > +
> > +	return nb_ops;
> > +}
> 
> [Fan: same naming issue - maybe rte_cryptodev_dequeue_post, so
> as the enqueue part]

Same comment as above. Aligned with ethdev and eventdev changes.

> 
> > +#define _RTE_CRYPTO_DEQ_FUNC(fn)	_rte_crypto_deq_##fn
> > +
> > +/**
> > + * @internal
> > + * Helper macro to create new API wrappers for existing PMD dequeue
> > functions.
> > + */
> > +#define _RTE_CRYPTO_DEQ_PROTO(fn) \
> > +	uint16_t _RTE_CRYPTO_DEQ_FUNC(fn)(uint8_t dev_id, uint8_t
> > qp_id, \
> > +			struct rte_crypto_op **ops, uint16_t nb_ops)
> > +
> > +/**
> > + * @internal
> > + * Helper macro to create new API wrappers for existing PMD dequeue
> > functions.
> > + */
> > +#define _RTE_CRYPTO_DEQ_DEF(fn) \
> > +_RTE_CRYPTO_DEQ_PROTO(fn) \
> > +{ \
> > +	void *qp = _rte_cryptodev_dequeue_prolog(dev_id, qp_id); \
> > +	if (qp == NULL) \
> [Fan: suggest to add "unlikely" above]

Ok