Hi Jerin, Thanks for the review. Please find few comments inline.
> -----Original Message----- > From: Jerin Jacob [mailto:jerin.ja...@caviumnetworks.com] > Sent: Saturday, February 17, 2018 1:04 AM > To: Gujjar, Abhinandan S <abhinandan.guj...@intel.com> > Cc: dev@dpdk.org; Vangati, Narender <narender.vang...@intel.com>; Rao, > Nikhil <nikhil....@intel.com>; Eads, Gage <gage.e...@intel.com>; > hemant.agra...@nxp.com; akhil.go...@nxp.com; > narayanaprasad.athr...@cavium.com; nidadavolu.mur...@cavium.com; > nithin.dabilpu...@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.guj...@intel.com> > > To: jerin.ja...@caviumnetworks.com > > CC: dev@dpdk.org, narender.vang...@intel.com, Abhinandan Gujjar > > <abhinandan.guj...@intel.com>, Nikhil Rao <nikhil....@intel.com>, Gage > > Eads <gage.e...@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.guj...@intel.com> > > Signed-off-by: Nikhil Rao <nikhil....@intel.com> > > Signed-off-by: Gage Eads <gage.e...@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