Re: [PATCH v5 1/3] lib: introduce dispatcher library

[David Marchand <david.marchand@redhat.com>]

Hello Mattias,

On Thu, Oct 5, 2023 at 12:09 PM Mattias Rönnblom <hofors@lysator.liu.se> wrote:
> >> +
> >> +deps += ['eventdev']
> >> diff --git a/lib/dispatcher/rte_dispatcher.c b/lib/dispatcher/rte_dispatcher.c
> >> new file mode 100644
> >> index 0000000000..0e69db2b9b
> >> --- /dev/null
> >> +++ b/lib/dispatcher/rte_dispatcher.c
> >> @@ -0,0 +1,708 @@
> >> +/* SPDX-License-Identifier: BSD-3-Clause
> >> + * Copyright(c) 2023 Ericsson AB
> >> + */
> >> +
> >> +#include <stdbool.h>
> >> +#include <stdint.h>
> >> +
> >> +#include <rte_branch_prediction.h>
> >> +#include <rte_common.h>
> >> +#include <rte_lcore.h>
> >> +#include <rte_random.h>
> >> +#include <rte_service_component.h>
> >> +
> >> +#include "eventdev_pmd.h"
> >> +
> >> +#include <rte_dispatcher.h>
> >> +
> >> +#define EVD_MAX_PORTS_PER_LCORE 4
> >> +#define EVD_MAX_HANDLERS 32
> >> +#define EVD_MAX_FINALIZERS 16
> >> +#define EVD_AVG_PRIO_INTERVAL 2000
> >> +#define EVD_SERVICE_NAME "dispatcher"
> >> +
> >> +struct rte_dispatcher_lcore_port {
> >> +       uint8_t port_id;
> >> +       uint16_t batch_size;
> >> +       uint64_t timeout;
> >> +};
> >> +
> >> +struct rte_dispatcher_handler {
> >> +       int id;
> >> +       rte_dispatcher_match_t match_fun;
> >> +       void *match_data;
> >> +       rte_dispatcher_process_t process_fun;
> >> +       void *process_data;
> >> +};
> >> +
> >> +struct rte_dispatcher_finalizer {
> >> +       int id;
> >> +       rte_dispatcher_finalize_t finalize_fun;
> >> +       void *finalize_data;
> >> +};
> >> +
> >> +struct rte_dispatcher_lcore {
> >> +       uint8_t num_ports;
> >> +       uint16_t num_handlers;
> >> +       int32_t prio_count;
> >> +       struct rte_dispatcher_lcore_port ports[EVD_MAX_PORTS_PER_LCORE];
> >> +       struct rte_dispatcher_handler handlers[EVD_MAX_HANDLERS];
> >> +       struct rte_dispatcher_stats stats;
> >> +} __rte_cache_aligned;
> >> +
> >> +struct rte_dispatcher {
> >> +       uint8_t event_dev_id;
> >> +       int socket_id;
> >> +       uint32_t service_id;
> >> +       struct rte_dispatcher_lcore lcores[RTE_MAX_LCORE];
> >> +       uint16_t num_finalizers;
> >> +       struct rte_dispatcher_finalizer finalizers[EVD_MAX_FINALIZERS];
> >> +};
> >> +
> >> +static int
> >> +evd_lookup_handler_idx(struct rte_dispatcher_lcore *lcore,
> >> +                      const struct rte_event *event)
> >
> > Wrt DPDK coding tyle, indent is a single tab.
> > Adding an extra tab is recommended when continuing control statements
> > like if()/for()/..
> >
>
> Sure, but I don't understand why you mention this.

I wanted to remind the DPDK coding style which I try to more closely
enforce for new code.
indent is off in this file (especially for function prototypes with
multiple tabs used).

>
> > On the other hand, max accepted length for a line is 100 columns.
> >
> > Wdyt of a single line for this specific case?
>
>
> Are you asking why the evd_lookup_handler_idx() function prototype is
> not a single line?
>
> It would make it long, that's why. Even if 100 wide lines are allowed,
> it doesn't means the author is forced to use such long lines?

I find it more readable.
If you want to stick to 80 columns, please comply with a single tab for indent.

[snip]


> >> +static int
> >> +evd_set_service_runstate(struct rte_dispatcher *dispatcher, int state)
> >> +{
> >> +       int rc;
> >> +
> >> +       rc = rte_service_component_runstate_set(dispatcher->service_id,
> >> +                                               state);
> >> +
> >> +       if (rc != 0) {
> >> +               RTE_EDEV_LOG_ERR("Unexpected error %d occurred while setting "
> >> +                                "service component run state to %d\n", rc,
> >> +                                state);
> >> +               RTE_ASSERT(0);
> >
> > Why not propagating the error to callers?
> >
> >
>
> The root cause would be a programming error, hence an assertion is more
> appropriate way to deal with the situation.

Without building RTE_ENABLE_ASSERT (disabled by default), the code
later in this function will still be executed.

[snip]


> >> +typedef void
> >> +(*rte_dispatcher_finalize_t)(uint8_t event_dev_id, uint8_t event_port_id,
> >> +                                  void *cb_data);
> >> +
> >> +/**
> >> + * Dispatcher statistics
> >> + */
> >> +struct rte_dispatcher_stats {
> >> +       uint64_t poll_count;
> >> +       /**< Number of event dequeue calls made toward the event device. */
> >
> > We had a number of issues with doxygen post annotations.
> > Prefer the prefixed ones.
> >
>
> OK. More readable, too. I just used the postfix syntax since it seemed
> the only one used in DPDK.

Historically yes, but we started cleaning headers for readability
(like in ethdev) and after catching a few errors with postfix
comments.


-- 
David Marchand