From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 6100A4233F; Mon, 9 Oct 2023 18:49:58 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id E94C14068E; Mon, 9 Oct 2023 18:49:57 +0200 (CEST) Received: from mail.lysator.liu.se (mail.lysator.liu.se [130.236.254.3]) by mails.dpdk.org (Postfix) with ESMTP id 14F75402B1; Mon, 9 Oct 2023 18:49:56 +0200 (CEST) Received: from mail.lysator.liu.se (localhost [127.0.0.1]) by mail.lysator.liu.se (Postfix) with ESMTP id 893042623E; Mon, 9 Oct 2023 18:49:55 +0200 (CEST) Received: by mail.lysator.liu.se (Postfix, from userid 1004) id 7D59D2605E; Mon, 9 Oct 2023 18:49:55 +0200 (CEST) X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on hermod.lysator.liu.se X-Spam-Level: X-Spam-Status: No, score=-1.5 required=5.0 tests=ALL_TRUSTED,AWL autolearn=disabled version=3.4.6 X-Spam-Score: -1.5 Received: from [192.168.1.59] (h-62-63-215-114.A163.priv.bahnhof.se [62.63.215.114]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-256) server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mail.lysator.liu.se (Postfix) with ESMTPSA id 496B4260F9; Mon, 9 Oct 2023 18:49:54 +0200 (CEST) Message-ID: <909e0363-365a-4250-85ba-43c8715642a3@lysator.liu.se> Date: Mon, 9 Oct 2023 18:49:53 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v5 1/3] lib: introduce dispatcher library To: David Marchand Cc: =?UTF-8?Q?Mattias_R=C3=B6nnblom?= , dev@dpdk.org, Jerin Jacob , techboard@dpdk.org, harry.van.haaren@intel.com, Peter Nilsson , Heng Wang , Naga Harish K S V , Pavan Nikhilesh , Gujjar Abhinandan S , Erik Gabriel Carrillo , Shijith Thotton , Hemant Agrawal , Sachin Saxena , Liang Ma , Peter Mccarthy , Zhirun Yan References: <20230922073825.351453-2-mattias.ronnblom@ericsson.com> <20230928073056.359356-1-mattias.ronnblom@ericsson.com> <20230928073056.359356-2-mattias.ronnblom@ericsson.com> <0c8eaf05-8dc7-4462-a893-2b7019d24fae@lysator.liu.se> Content-Language: en-US From: =?UTF-8?Q?Mattias_R=C3=B6nnblom?= In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Virus-Scanned: ClamAV using ClamSMTP X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On 2023-10-06 10:46, David Marchand wrote: > Hello Mattias, > > On Thu, Oct 5, 2023 at 12:09 PM Mattias Rönnblom 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 >>>> +#include >>>> + >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> + >>>> +#include "eventdev_pmd.h" >>>> + >>>> +#include >>>> + >>>> +#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. > >