Re: [dpdk-dev] [PATCH v5] crypto/scheduler: add driver for scheduler crypto pmd

[Thomas Monjalon <thomas.monjalon@6wind.com>]

2017-01-17 14:09, Declan Doherty:
> On 17/01/17 13:19, Fan Zhang wrote:
> > This patch provides the initial implementation of the scheduler poll mode
> > driver using DPDK cryptodev framework.
> >
> > Scheduler PMD is used to schedule and enqueue the crypto ops to the
> > hardware and/or software crypto devices attached to it (slaves). The
> > dequeue operation from the slave(s), and the possible dequeued crypto op
> > reordering, are then carried out by the scheduler.
> >
> > As the initial version, the scheduler PMD currently supports only the
> > Round-robin mode, which distributes the enqueued burst of crypto ops
> > among its slaves in a round-robin manner. This mode may help to fill
> > the throughput gap between the physical core and the existing cryptodevs
> > to increase the overall performance. Moreover, the scheduler PMD is
> > provided the APIs for user to create his/her own scheduler.
> >
> > Build instructions:
> > To build DPDK with CRYTPO_SCHEDULER_PMD the user is required to set
> > CONFIG_RTE_LIBRTE_PMD_CRYPTO_SCHEDULER=y in config/common_base
> >
> > Notice:
> > - Scheduler PMD shares same EAL commandline options as other cryptodevs.
> >   However, apart from socket_id, the rest of cryptodev options are
> >   ignored. The scheduler PMD's max_nb_queue_pairs and max_nb_sessions
> >   options are set as the minimum values of the attached slaves'. For
> >   example, a scheduler cryptodev is attached 2 cryptodevs with
> >   max_nb_queue_pairs of 2 and 8, respectively. The scheduler cryptodev's
> >   max_nb_queue_pairs will be automatically updated as 2.
> >
> > - In addition, an extra option "slave" is added. The user can attach one
> >   or more slave cryptodevs initially by passing their names with this
> >   option. Here is an example:
> >
> >   ... --vdev "crypto_aesni_mb_pmd,name=aesni_mb_1" --vdev "crypto_aesni_
> >   mb_pmd,name=aesni_mb_2" --vdev "crypto_scheduler_pmd,slave=aesni_mb_1,
> >   slave=aesni_mb_2" ...
> >
> >   Remember the software cryptodevs to be attached shall be declared before
> >   the scheduler PMD, otherwise the scheduler will fail to locate the
> >   slave(s) and report error.
> >
> > - The scheduler cryptodev cannot be started unless the scheduling mode
> >   is set and at least one slave is attached. Also, to configure the
> >   scheduler in the run-time, like attach/detach slave(s), change
> >   scheduling mode, or enable/disable crypto op ordering, one should stop
> >   the scheduler first, otherwise an error will be returned.
> >
> > Changes in v5:
> > Fixed EOF whitespace warning.
> > Updated Copyright.
> >
> > Changes in v4:
> > Fixed a few bugs.
> > Added slave EAL commandline option support.
> >
> > Changes in v3:
> > Fixed config/common_base.
> >
> > Changes in v2:
> > New approaches in API to suit future scheduling modes.
> >
> > Signed-off-by: Fan Zhang <roy.fan.zhang@intel.com>
> > Signed-off-by: Declan Doherty <declan.doherty@intel.com>
> > ---
> >  config/common_base                                 |   6 +
> >  drivers/crypto/Makefile                            |   1 +
> >  drivers/crypto/scheduler/Makefile                  |  66 +++
> >  drivers/crypto/scheduler/rte_cryptodev_scheduler.c | 460 +++++++++++++++++++
> >  drivers/crypto/scheduler/rte_cryptodev_scheduler.h | 167 +++++++
> >  .../scheduler/rte_cryptodev_scheduler_operations.h |  71 +++
> >  .../scheduler/rte_pmd_crypto_scheduler_version.map |  12 +
> >  drivers/crypto/scheduler/scheduler_pmd.c           | 360 +++++++++++++++
> >  drivers/crypto/scheduler/scheduler_pmd_ops.c       | 489 +++++++++++++++++++++
> >  drivers/crypto/scheduler/scheduler_pmd_private.h   | 115 +++++
> >  drivers/crypto/scheduler/scheduler_roundrobin.c    | 417 ++++++++++++++++++
> >  lib/librte_cryptodev/rte_cryptodev.h               |   4 +
> >  mk/rte.app.mk                                      |   3 +-
> >  13 files changed, 2170 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/crypto/scheduler/Makefile
> >  create mode 100644 drivers/crypto/scheduler/rte_cryptodev_scheduler.c
> >  create mode 100644 drivers/crypto/scheduler/rte_cryptodev_scheduler.h
> >  create mode 100644 drivers/crypto/scheduler/rte_cryptodev_scheduler_operations.h
> >  create mode 100644 drivers/crypto/scheduler/rte_pmd_crypto_scheduler_version.map
> >  create mode 100644 drivers/crypto/scheduler/scheduler_pmd.c
> >  create mode 100644 drivers/crypto/scheduler/scheduler_pmd_ops.c
> >  create mode 100644 drivers/crypto/scheduler/scheduler_pmd_private.h
> >  create mode 100644 drivers/crypto/scheduler/scheduler_roundrobin.c
[...]
> 
> Acked-by: Declan Doherty <declan.doherty@intel.com>

NACK
I could argue it is too big for an unique patch,
but it's even worst when you ack without stripping the long patch.
My mouse is out of order after this long scroll looking for a comment.