Re: [dpdk-dev] [RFC v2, 2/2] eventdev: add crypto adapter API header

["Gujjar, Abhinandan S" <abhinandan.gujjar@intel.com>]

Hi Jerin,

Thanks for the review. Please find few comments inline.

> -----Original Message-----
> From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> Sent: Saturday, February 17, 2018 1:04 AM
> To: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>
> Cc: dev@dpdk.org; Vangati, Narender <narender.vangati@intel.com>; Rao,
> Nikhil <nikhil.rao@intel.com>; Eads, Gage <gage.eads@intel.com>;
> hemant.agrawal@nxp.com; akhil.goyal@nxp.com;
> narayanaprasad.athreya@cavium.com; nidadavolu.murthy@cavium.com;
> nithin.dabilpuram@cavium.com
> Subject: Re: [RFC v2, 2/2] eventdev: add crypto adapter API header
> 
> -----Original Message-----
> > Date: Mon, 15 Jan 2018 16:23:50 +0530
> > From: Abhinandan Gujjar <abhinandan.gujjar@intel.com>
> > To: jerin.jacob@caviumnetworks.com
> > CC: dev@dpdk.org, narender.vangati@intel.com, Abhinandan Gujjar
> > <abhinandan.gujjar@intel.com>, Nikhil Rao <nikhil.rao@intel.com>, Gage
> > Eads <gage.eads@intel.com>
> > Subject: [RFC v2, 2/2] eventdev: add crypto adapter API header
> > X-Mailer: git-send-email 1.9.1
> >
> > Add crypto event adapter APIs to support packet transfer mechanism
> > between cryptodev and event device.
> >
> > Signed-off-by: Abhinandan Gujjar <abhinandan.gujjar@intel.com>
> > Signed-off-by: Nikhil Rao <nikhil.rao@intel.com>
> > Signed-off-by: Gage Eads <gage.eads@intel.com>
> > ---
> 
> Overall it looks good to me. Though I have some confusion over ENQ-DEQ
> scheme. May be it will be get cleared along with implementation.
> 
> > Notes:
> > 	V2:
> > 	1. Updated type as ENQ-DEQ in rte_event_crypto_adapter_type
> > 	2. Removed enum rte_event_crypto_conf_type
> > 	3. Updated struct rte_event_crypto_metadata
> > 	4. Removed struct rte_event_crypto_queue_pair_conf
> > 	5. Updated rte_event_crypto_adapter_queue_pair_add() API
> >
> >  lib/librte_eventdev/Makefile                   |   1 +
> >  lib/librte_eventdev/rte_event_crypto_adapter.h | 452
> > +++++++++++++++++++++++++
> 
> 1) Please update MAINTAINERS file
> 2) Add meson build support
> 3) Update doc/api/doxy-api-index.md file and check the doxygen output using
> "make doc-api-html"
> 4) Please add the programmers guide in doc/guides/prog_guide/
Sure
> 
> >
> > +++ b/lib/librte_eventdev/rte_event_crypto_adapter.h
> > @@ -0,0 +1,452 @@
> > +/*
> > + *   Copyright(c) 2018 Intel Corporation. All rights reserved.
> > + *   All rights reserved.
> > + *
> > + *   Redistribution and use in source and binary forms, with or without
> 
> Use SPDX tag scheme
Ok
> 
> > +#ifndef _RTE_EVENT_CRYPTO_ADAPTER_
> > +#define _RTE_EVENT_CRYPTO_ADAPTER_
> > +
> > +/**
> > + * This adapter adds support to enqueue crypto completions to event device.
> > + * The packet flow from cryptodev to the event device can be
> > +accomplished
> > + * using both SW and HW based transfer mechanisms.
> > + * The adapter uses a EAL service core function for SW based packet
> > +transfer
> > + * and uses the eventdev PMD functions to configure HW based packet
> > +transfer
> > + * between the cryptodev and the event device.
> > + *
> > + * In the case of SW based transfers, application can choose to
> > +submit a
> 
> I think, we can remove "In the case of SW based transfers" as it should be
> applicable for HW case too
Ok. In that case, adapter will detect the presence of HW connection between
cryptodev & eventdev and will not dequeue crypto completions.

> 
> > + * crypto operation directly to cryptodev or send it  to the
> > + cryptodev
> > + * adapter via eventdev, the cryptodev adapter then submits the
> > + crypto
> > + * operation to the crypto device. The first mode is known as the
> 
> The first mode (DEQ) is very clear. In the second mode(ENQ_DEQ),
> - How does "worker" submits the crypto work through crypto-adapter?
> If I understand it correctly, "workers" always deals with only cryptodev's
> rte_cryptodev_enqueue_burst() API and "service" function in crypto adapter
> would be responsible for dequeue() from cryptodev and enqueue to eventdev?
> 
> I understand the need for OP_NEW vs OP_FWD mode difference in both modes.
> Other than that, What makes ENQ_DEQ different? Could you share the flow for
> ENQ_DEQ mode with APIs.

/*
Application changes for ENQ_DEQ mode:
-------------------------------------------------
	/* In ENQ_DEQ mode, to enqueue to adapter app
	 * has to fill out following details.
	 */
	struct rte_event_crypto_request *req;
	struct rte_crypto_op *op = rte_crypto_op_alloc();
	
	/* fill request info */
	req = (void *)((char *)op + op.private_data_offset);
	req->cdev_id = 1;
	req->queue_pair_id = 1;

	/* fill response info */
	...

	/* send event to crypto adapter */
	ev->event_ptr = op;
	ev->queue_id = dst_event_qid;
	ev->priority = dst_priority;
	ev->sched_type = dst_sched_type;
	ev->event_type = RTE_EVENT_TYPE_CRYPTODEV;
	ev->sub_event_type = sub_event_type;
	ev->flow_id = dst_flow_id;
	ret = rte_event_enqueue_burst(event_dev_id, event_port_id, ev, 1);


Adapter in ENQ_DEQ mode, submitting crypto ops to cryptodev:
-----------------------------------------------------------------------------
	n = rte_event_dequeue_burst(event_dev_id, event_port_id, ev, BATCH_SIZE, time_out);
	struct rte_crypto_op *op = ev->event_ptr;
	struct rte_event_crypto_request *req = (void *)op + op.private_data_offset;
	cdev_id = req->cdev_id;
	qp_id = req->queue_pair_id

	ret = rte_cryptodev_enqueue_burst(cdev_id, qp_id, op, 1);
*/
> 
> > + * dequeue only (DEQ) mode  and the second as the enqueue - dequeue
> 
> extra space between "mode" and "and"
Ok
> 
> > + * (ENQ_DEQ) mode. The choice of mode can be specified when creating
> > + * the adapter.
> > + * In the latter choice, the cryptodev adapter is able to use
> > + * RTE_OP_FORWARD as the event dev enqueue type, this has a
> > + performance
> > + * advantage in "closed system" eventdevs like the eventdev SW PMD
> > + and
> 
> s/eventdevs/eventdev ??
Ok
> 
> > + * also, the event cannot be dropped.
> > + *
> > + * In the ENQ_DEQ mode the application needs to specify the cryptodev
> > + ID
> > + * and queue pair ID (request information) in addition to the event
> > + * information (response information) needed to enqueue an event
> > + after
> > + * the crypto operation has completed. The request and response
> > + information
> > + * are specified in the rte_crypto_op private_data. In the DEQ mode
> > + the
> > + * the application is required to provide only the response information.
> 
> Why ENQ_DEQ modes needs cryptodev ID and queue pair ID? Which is part of
> rte_cryptodev_enqueue_burst(). May be I am missing something.
> 
> If ENQ_DEQ modes help in SW case then we can add it as capability.
> I am just trying to see any scope of convergence.
In ENQ_DEQ mode, adapter dequeues eventdev and dereferences cryptodev ID and queue pair ID
to enqueue the crypto operations to cryptodev using rte_cryptodev_enqueue_burst() API.
I hope, above code snippet would have made things more clear.
> 
> > + *
> > + * In the ENQ_DEQ mode, application sends crypto operations as events
> > + to
> > + * the adapter which dequeues events and programs cryptodev operations.
> > + * The adapter then dequeues crypto completions from cryptodev and
> > + * enqueue events to the event device.
> > + *
> > + * In the case of HW based transfers, the cryptodev PMD callback
> > + invoked
> > + * from rte_cryptodev_enqueue_burst() uses the response information
> > + to
> > + * setup the event for the cryptodev completion.
> > + *
> > + * The event crypto adapter provides common APIs to configure the
> > + packet flow
> > + * from the cryptodev to event devices across both SW and HW based
> transfers.
> > + * The crypto event adapter's functions are:
> > + *  - rte_event_crypto_adapter_create_ext()
> > + *  - rte_event_crypto_adapter_create()
> > + *  - rte_event_crypto_adapter_free()
> > + *  - rte_event_crypto_adapter_queue_pair_add()
> > + *  - rte_event_crypto_adapter_queue_pair_del()
> > + *  - rte_event_crypto_adapter_start()
> > + *  - rte_event_crypto_adapter_stop()
> > + *  - rte_event_crypto_adapter_stats_get()
> > + *  - rte_event_crypto_adapter_stats_reset()
> > +
> > + * The applicaton creates an instance using
> > + rte_event_crypto_adapter_create()
> 
> s/applicaton/application
Ok
> 
> > + * or rte_event_crypto_adapter_create_ext().
> > + *
> > + * Cryptodev queue pair addition/deletion is done
> > + * using the rte_event_crypto_adapter_queue_pair_xxx() APIs.
> > + *
> > + * The SW adapter or HW PMD uses rte_crypto_op::private_data_type to
> > + decide
> > + * whether request/response data is located in the crypto
> > + session/crypto
> > + * security session or at an offset in the rte_crypto_op.
> > + * rte_crypto_op::private_data_offset is used to locate the
> > + request/response
> > + * in the rte_crypto_op. If the rte_crypto_op::private_data_type
> > + * indicates that the data is in the crypto session/crypto security
> > + session
> > + * then the rte_crypto_op::sess_type is used to decide whether the
> > + private
> > + * data is in the session or security session.
> > + *
> > + * For session-less it is mandatory to place the request/response
> > + data with
> > + * the rte_crypto_op where as with crypto session/security session it
> > + can be
> > + * placed with the rte_crypto_op or in the session/security session.
> 
> Please update(if needed) above two paragraphs based on the conclusion from
> cryptodev change
Sure
> 
> > + */
> > +
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif
> > +
> > +#include <stdint.h>
> > +#include <rte_service.h>
> > +
> > +#include "rte_eventdev.h"
> > +
> > +#define RTE_EVENT_CRYPTO_ADAPTER_MAX_INSTANCE 32
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this enum may change without prior notice
> 
> Change to new EXPERIMENTAL tag scheme.
Ok
> 
> > + *
> > + * Crypto event adapter type
> > + */
> > +enum rte_event_crypto_adapter_type {
> > +	RTE_EVENT_CRYPTO_ADAPTER_DEQ_ONLY = 1,
> > +	/**< Start only dequeue part of crypto adapter.
> > +	 * Packets dequeued from cryptodev are enqueued to eventdev
> > +	 * as new events and events will be treated as RTE_EVENT_OP_NEW. */
> > +	RTE_EVENT_CRYPTO_ADAPTER_ENQ_DEQ,
> > +	/**< Start both enqueue & dequeue part of crypto adapter.
> > +	 * Packet's event context will be retained and
> > +	 * event will be treated as RTE_EVENT_OP_FORWARD. */
> 
> This description makes sense.
> 
> > +};
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this structure may change without prior notice
> > + *
> > + * Crypto event request structure will be fill by application to
> > + * provide event request information to the adapter.
> > + */
> > +struct rte_event_crypto_request {
> > +	uint8_t resv[8];
> > +	/**< Overlaps with first 8 bytes of struct rte_event
> > +	 * that encode the response event information
> > +	 */
> > +	uint16_t cdev_id;
> > +	/**< cryptodev ID to be used */
> > +	uint16_t queue_pair_id;
> > +	/**< cryptodev queue pair ID to be used */
> 
> Shouldn't we say it is need only with ENQ_DEQ mode as described earlier.
Yes. I will update it.
> adding "@see" section would help
Ok
> 
> > +	uint32_t resv1;
> > +	/**< Reserved bits */
> > +};
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this structure may change without prior notice
> > + *
> > + * Crypto event metadata structure will be filled by application
> > + * to provide crypto request and event response information.
> > + *
> > + * If crypto events are enqueued using a HW mechanism, the cryptodev
> > + * PMD will use the event response information to set up the event
> > + * that is enqueued back to eventdev after completion of the crypto
> > + * operation. If the transfer is done by SW, it will be used by the
> > + * adapter.
> > + */
> > +union rte_event_crypto_metadata {
> > +	struct rte_event_crypto_request request_info;
> > +	struct rte_event response_info;
> 
> Perfect.
> 
> Other than above comments, everything else looks OK to me.

-Abhinandan