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

["Mattias Rönnblom" <hofors@lysator.liu.se>]

On 2023-10-06 10:46, David Marchand wrote:
> 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).
> 

I just didn't understand what rule I was breaking, but I see now.

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

If RTE_ASSERT() is not the way to assure a consistent internal library 
state, what is? RTE_VERIFY()?

A side note: in the DPDK code base, the majority of 
rte_service_component_runstate_set() calls ignore the return value. No 
big surprise, since in many cases this error can't happen with less than 
the internal state being inconsistent.

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