Re: [RFC 1/1] eventdev: add distributed software (DSW) event device

[Jerin Jacob <jerin.jacob@caviumnetworks.com>]

-----Original Message-----
> Date: Sun, 22 Jul 2018 17:02:54 +0530
> From: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> To: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
> Cc: dev@dpdk.org, bruce.richardson@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.ronnblom@ericsson.com>
> > To: dev@dpdk.org
> > CC: jerin.jacob@caviumnetworks.com, bruce.richardson@intel.com, Mattias
> >  Rönnblom <mattias.ronnblom@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.ronnblom@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
>