-----Original Message-----
> Date: Sun, 22 Jul 2018 17:02:54 +0530
> From: Jerin Jacob <jerin.ja...@caviumnetworks.com>
> To: Mattias Rönnblom <mattias.ronnb...@ericsson.com>
> Cc: dev@dpdk.org, bruce.richard...@intel.com
> Subject: Re: [RFC 1/1] eventdev: add distributed software (DSW) event device
> User-Agent: Mutt/1.10.1 (2018-07-13)
>
> -----Original Message-----
> > Date: Wed, 11 Jul 2018 23:08:44 +0200
> > From: Mattias Rönnblom <mattias.ronnb...@ericsson.com>
> > To: dev@dpdk.org
> > CC: jerin.ja...@caviumnetworks.com, bruce.richard...@intel.com, Mattias
> > Rönnblom <mattias.ronnb...@ericsson.com>
> > Subject: [RFC 1/1] eventdev: add distributed software (DSW) event device
> > X-Mailer: git-send-email 2.17.1
> >
> >
> > Signed-off-by: Mattias Rönnblom <mattias.ronnb...@ericsson.com>
>
> Thanks Mattias for a yet another eventdev driver.
> Since it can work as distributed scheduler, it is complementary to existing
> SW scheduler.
>
> A few comments:
> 1) There is compilation error in my setup
> /export/dpdk-next-eventdev/drivers/event/dsw/dsw_event.c: In function
> ‘dsw_port_consider_migration’:
> /export/dpdk-next-eventdev/drivers/event/dsw/dsw_event.c:346:39: error:
> ‘current_burst’ may be used uninitialized in this function
> [-Werror=maybe-uninitialized]
> ((int)((((_qf)->queue_id)<<16)|((_qf)->flow_hash)))
>
> 2) Please split the patch as more logical one.s You see the git history
> of driver/event/sw to get idea on the patch split.
>
> 3) Please update the documentation @ doc/guides/eventdevs/
I would like to include this driver for v18.08. Please send the next
the version in order to complete review before giving RC1 pull request.
>
> > ---
> > config/common_base | 5 +
> > drivers/event/Makefile | 1 +
>
> meson build support is missing.
>
> > drivers/event/dsw/Makefile | 28 +
> > drivers/event/dsw/dsw_evdev.c | 361 +++++
> > drivers/event/dsw/dsw_evdev.h | 296 ++++
> > drivers/event/dsw/dsw_event.c | 1285 +++++++++++++++++
> > drivers/event/dsw/dsw_sort.h | 47 +
> > drivers/event/dsw/dsw_xstats.c | 284 ++++
> > .../event/dsw/rte_pmd_evdev_dsw_version.map | 3 +
> > mk/rte.app.mk | 1 +
> > 10 files changed, 2311 insertions(+)
> > index 000000000..39008f7eb
> > --- /dev/null
> > +++ b/drivers/event/dsw/dsw_evdev.c
> > @@ -0,0 +1,361 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(c) 2018 Ericsson AB
> > + */
> > +
> > +#include <rte_eventdev_pmd.h>
> > +#include <rte_eventdev_pmd_vdev.h>
> > +#include <rte_cycles.h>
> > +#include <rte_random.h>
>
> sort it in alphabetical order.
>
> > +
> > +#include "dsw_evdev.h"
> > +
> > +static int
> > +dsw_start(struct rte_eventdev *dev)
> > +{
> > + struct dsw_evdev *dsw = dsw_pmd_priv(dev);
> > + uint16_t i;
> > + uint64_t now;
> > +
> > + rte_atomic32_init(&dsw->credits_on_loan);
> > +
> > + initial_flow_to_port_assignment(dsw);
> > +
> > + now = rte_get_timer_cycles();
> > + for (i = 0; i < dsw->num_ports; i++) {
> > + dsw->ports[i].measurement_start = now;
> > + dsw->ports[i].busy_start = now;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void
> > +dsw_stop(struct rte_eventdev *dev __rte_unused)
> > +{
>
> You may implement, eventdev_stop_flush_t callback to free up the
> outstanding events in the eventdev.
>
>
> > +}
> > +
> > +static int
> > +dsw_close(struct rte_eventdev *dev)
> > +{
> > + struct dsw_evdev *dsw = dsw_pmd_priv(dev);
> > +
> > + dsw->num_ports = 0;
> > + dsw->num_queues = 0;
> > +
> > + return 0;
> > +}
> > +
> > +static struct rte_eventdev_ops dsw_evdev_ops = {
> > + .dev_infos_get = dsw_info_get,
> > + .dev_configure = dsw_configure,
> > + .dev_start = dsw_start,
> > + .dev_stop = dsw_stop,
> > + .dev_close = dsw_close,
> > + .port_setup = dsw_port_setup,
> > + .port_def_conf = dsw_port_def_conf,
> > + .port_release = dsw_port_release,
> > + .queue_setup = dsw_queue_setup,
> > + .queue_def_conf = dsw_queue_def_conf,
> > + .queue_release = dsw_queue_release,
> > + .port_link = dsw_port_link,
> > + .port_unlink = dsw_port_unlink,
> > +#ifdef DSW_XSTATS
>
> Instead of creating a lot of "DSW_XSTATS" defines in this file, How about
> creating NOP version of xstats functions when ifndef DSW_XSTATS as
> different file? So that #ifdef clutter will go way.
>
>
> > + .xstats_get = dsw_xstats_get,
> > + .xstats_get_names = dsw_xstats_get_names,
> > + .xstats_get_by_name = dsw_xstats_get_by_name
> > +#endif
> > +};
> > +
> > +static int
> > +dsw_probe(struct rte_vdev_device *vdev)
> > +{
> > + const char *name;
> > + struct rte_eventdev *dev;
> > + struct dsw_evdev *dsw;
> > +
> > + name = rte_vdev_device_name(vdev);
> > +
>
> Please add secondary process check. See SW driver.
>
> > +RTE_PMD_REGISTER_VDEV(EVENTDEV_NAME_DSW_PMD, evdev_dsw_pmd_drv);
> > diff --git a/drivers/event/dsw/dsw_evdev.h b/drivers/event/dsw/dsw_evdev.h
> > new file mode 100644
> > index 000000000..e6b34c013
> > --- /dev/null
> > +++ b/drivers/event/dsw/dsw_evdev.h
> > @@ -0,0 +1,296 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(c) 2018 Ericsson AB
> > + */
> > +
> > +#ifndef _DSW_EVDEV_H_
> > +#define _DSW_EVDEV_H_
> > +
> > +#include <inttypes.h>
> > +#include <stdbool.h>
> > +#include <rte_eventdev.h>
> > +#include <rte_event_ring.h>
> > +#include <rte_ring.h>
> > +#include <rte_event_ring.h>
> > +#include <rte_config.h>
>
> sort it in alphabetical order.
>
> > +
> > +#define DSW_PMD_NAME RTE_STR(event_dsw)
> > +
> > +
> > +/* Only one outstanding migration per port is allowed */
> > +#define DSW_MAX_PAUSED_FLOWS (DSW_MAX_PORTS)
> > +
> > +/* Enough room for paus request/confirm and unpaus request/confirm for
> > + * all possible senders.
> > + */
> > +#define DSW_CTL_IN_RING_SIZE ((DSW_MAX_PORTS-1)*4)
> > +
> > +/* Statistics is configurable at this point, mostly to make it easy to
> > + * measure their performance impact.
> > + */
> > +#define DSW_XSTATS
>
> It can be moved to main config area.
>
> > +
> > +/* With DSW_SORT_DEQUEUED enabled, the scheduler will, at the point of
> > + * dequeue(), arrange events so that events with the same flow id on
> > + * the same queue forms a back-to-back "burst", and also so that such
> > + * bursts of different flow ids, but on the same queue, also come
> > + * consecutively. All this in an attempt to improve data and
> > + * instruction cache usage for the application, at the cost of a
> > + * scheduler overhead increase.
> > + */
> > +
> > +//#define DSW_SORT_DEQUEUED
>
> Checkpatch will complain about cpp style of comments.
>
> > +
> > +struct dsw_queue_flow {
> > + uint8_t queue_id;
> > + uint16_t flow_hash;
> > +};
> > +
> > +enum dsw_migration_state {
> > + DSW_MIGRATION_STATE_IDLE = 0,
>
> zero assignment is unnecessary.
>
> > + DSW_MIGRATION_STATE_PAUSING,
> > + DSW_MIGRATION_STATE_FORWARDING,
> > + DSW_MIGRATION_STATE_UNPAUSING
> > +};
> > +
> > +
> > + if (!dsw_select_migration_target(dsw, source_port, bursts,
> > num_bursts,
> > + port_loads,
> > + DSW_MIN_SOURCE_LOAD_FOR_MIGRATION,
> > + &source_port->migration_target_qf,
> > +
> > &source_port->migration_target_port_id)
> > + &&
> > + !dsw_select_migration_target(dsw, source_port, bursts,
> > num_bursts,
> > + port_loads,
> > + DSW_MAX_TARGET_LOAD_FOR_MIGRATION,
> > + &source_port->migration_target_qf,
> > +
> > &source_port->migration_target_port_id))
> > + return;
> > +
> > + DSW_LOG_DP_PORT(DEBUG, source_port->id, "Migrating queue_id %d "
> > + "flow_hash %d from port %d to port %d.\n",
> > + source_port->migration_target_qf.queue_id,
> > + source_port->migration_target_qf.flow_hash,
> > + source_port->id,
> > source_port->migration_target_port_id);
> > +#if 0
>
> Spotted #if 0
>
> > +#endif
> > diff --git a/drivers/event/dsw/rte_pmd_evdev_dsw_version.map
> > b/drivers/event/dsw/rte_pmd_evdev_dsw_version.map
> > new file mode 100644
> > index 000000000..ad6e191e4
> > --- /dev/null
> > +++ b/drivers/event/dsw/rte_pmd_evdev_dsw_version.map
> > @@ -0,0 +1,3 @@
> > +DPDK_18.08 {
>
> 18.11
>
