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

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

-----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/

> ---
>  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