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 BC11F42608; Thu, 21 Sep 2023 18:47:06 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id A8AEF4013F; Thu, 21 Sep 2023 18:47:06 +0200 (CEST) Received: from mail.lysator.liu.se (mail.lysator.liu.se [130.236.254.3]) by mails.dpdk.org (Postfix) with ESMTP id 52254400D7; Thu, 21 Sep 2023 18:47:05 +0200 (CEST) Received: from mail.lysator.liu.se (localhost [127.0.0.1]) by mail.lysator.liu.se (Postfix) with ESMTP id D9335130; Thu, 21 Sep 2023 18:47:04 +0200 (CEST) Received: by mail.lysator.liu.se (Postfix, from userid 1004) id CC8C2698; Thu, 21 Sep 2023 18:47:04 +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=-2.3 required=5.0 tests=ALL_TRUSTED, AWL, NICE_REPLY_A autolearn=disabled version=3.4.6 X-Spam-Score: -2.3 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)) (No client certificate requested) by mail.lysator.liu.se (Postfix) with ESMTPSA id 8F706557; Thu, 21 Sep 2023 18:47:02 +0200 (CEST) Message-ID: Date: Thu, 21 Sep 2023 18:47:02 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.15.1 From: =?UTF-8?Q?Mattias_R=c3=b6nnblom?= Subject: Re: [PATCH v3 1/3] lib: introduce dispatcher library To: Jerin Jacob , =?UTF-8?Q?Mattias_R=c3=b6nnblom?= Cc: 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 , =?UTF-8?Q?Morten_Br=c3=b8rup?= References: <20230616074041.159675-2-mattias.ronnblom@ericsson.com> <20230904130313.327809-1-mattias.ronnblom@ericsson.com> <20230904130313.327809-2-mattias.ronnblom@ericsson.com> Content-Language: en-US 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-09-19 12:58, Jerin Jacob wrote: > On Mon, Sep 4, 2023 at 6:39 PM Mattias Rönnblom > wrote: >> >> The purpose of the dispatcher library is to help reduce coupling in an >> Eventdev-based DPDK application. >> >> In addition, the dispatcher also provides a convenient and flexible >> way for the application to use service cores for application-level >> processing. >> >> Signed-off-by: Mattias Rönnblom >> Tested-by: Peter Nilsson > > High level architecture comment > -------------------------------- > > 1) I think, we don't need tie this library ONLY to event dev > application. It can be used with poll mode as well, > that way traditiona pipeline application with ethdev as source could > use this library dispatch the packets. > They could potentially use a library *like this*, but I'm not sure it should be this library, or at least I think it should be a different API (better type checking, plus no obvious benefit of being more generic). Another option for a traditional app calling rte_eth_rx_burst() directly is to start using eventdev. :) > We dont need to implement that first version but API can make room for > such abstractions. > > Based on my understanding in fast-path it has means to > a)Pull out the events using rte_event_dequeue() > b)Compare with registered match functions and call process upon match. > > if we abstract (a) as rte_dispatcher_source, We could pull from ethdev > via rte_eth_rx_burst() or > from ring via dequeue_burst API or so based on rte_dispatcher_source > selected for dispatch configuration > and we can use different sevice function pointers to have different service core > implementation without effecting performance each sources. > It could be generalized, for sure. I don't think it should be, at this point at least. Non-event dev events could - and at this point I'm leaning toward *should* - be consumed as a different DPDK service, but potentially on the same lcore. If you would want to prepare for a future with several different event sources, one could consider reintroducing the word "event" somewhere in the dispatcher's name. So you would have rte_event_dispatcher.h rte_eth_dispatcher.h or rte_dispatcher_event.h rte_dispatcher_eth.h > High level cosmetic comment > ---------------------------------------------------- > 1)Missing doxygen connection- See doc/api/doxy-api-index.md > rte_dispatcher.h is listed under **classification**, but this change is in the programming guide patch. I'll move it to the patch containing the header file. > Process related comment > ------------------------------------ > 1) Documentation does not need need separate patch. All recent library > changes documentation in same file. > You could have doc and API header file as first patch and > implementation as subsequent patches. > > I'm not sure how this is an improvement. Can you elaborate? For me, it just seems like a change. Are there some guidelines on how to split a larger change into a patch set? A section on this matter in the contribution guide would be great. > >> diff --git a/lib/dispatcher/rte_dispatcher.h b/lib/dispatcher/rte_dispatcher.h >> new file mode 100644 >> index 0000000000..6712687a08 >> --- /dev/null >> +++ b/lib/dispatcher/rte_dispatcher.h >> @@ -0,0 +1,480 @@ >> +/* SPDX-License-Identifier: BSD-3-Clause >> + * Copyright(c) 2023 Ericsson AB >> + */ >> + >> +#ifndef __RTE_DISPATCHER_H__ >> +#define __RTE_DISPATCHER_H__ >> + > > > All new API should be experimental. See > https://elixir.bootlin.com/dpdk/latest/source/lib/graph/rte_graph.h#L12 > example. > Noted. > >> +/** >> + * @file >> + * >> + * RTE Dispatcher >> + * >> + * The purpose of the dispatcher is to help decouple different parts >> + * of an application (e.g., modules), sharing the same underlying >> + * event device. >> + >> +/** >> + * Function prototype for match callbacks. >> + * >> + * Match callbacks are used by an application to decide how the >> + * dispatcher distributes events to different parts of the >> + * application. >> + * >> + * The application is not expected to process the event at the point >> + * of the match call. Such matters should be deferred to the process >> + * callback invocation. >> + * >> + * The match callback may be used as an opportunity to prefetch data. >> + * >> + * @param event >> + * Pointer to event >> + * >> + * @param cb_data >> + * The pointer supplied by the application in >> + * rte_dispatcher_register(). >> + * >> + * @return >> + * Returns true in case this events should be delivered (via >> + * the process callback), and false otherwise. >> + */ >> +typedef bool >> +(*rte_dispatcher_match_t)(const struct rte_event *event, void *cb_data); > > > a) Can we use void* event, so that it can be used with mbuf or other > type by casting in the call back implementer. > > b) I was thinking, How we can avoid this function pointer and enable > more have better performance at architecture level. > > Both x86, ARM has vector instructions[1] to form a vector from various > offset from memory and compare N events > in one shot. That is, if express match data like offset = X has value > is Y and offset = X has value = A. > I know, it may not good existing application using this APIs. But I > believe, it will be more performance > effective. If make sense, you can adapt to this.(Something to think about) > There may be a future development where you try to shave off a few of the circa 10 clock cycles per event of overhead that the current implementation incur. 10 cc is not a lot though. Your eventdev will need something like 10x that. With link-time optimizations, the matching function call will go away. I'm not sure how much auto-vectorization will happen. (The number quoted above is without LTO.) The event dispatcher as a separate library will result in an extra function call across a shared object boundary, which is not exactly for free. (The 10 cc are for static linking.) > > [1] > https://developer.arm.com/documentation/den0018/a/NEON-and-VFP-Instruction-Summary/NEON-general-data-processing-instructions/VTBL > >> + >> +/** >> + * Function prototype for process callbacks. >> + * >> + * The process callbacks are used by the dispatcher to deliver >> + * events for processing. >> + * >> + * @param event_dev_id >> + * The originating event device id. >> + * >> + * @param event_port_id >> + * The originating event port. >> + * >> + * @param events >> + * Pointer to an array of events. >> + * >> + * @param num >> + * The number of events in the @p events array. >> + * >> + * @param cb_data >> + * The pointer supplied by the application in >> + * rte_dispatcher_register(). >> + */ >> + >> +typedef void >> +(*rte_dispatcher_process_t)(uint8_t event_dev_id, uint8_t event_port_id, >> + struct rte_event *events, uint16_t num, >> + void *cb_data); > > Same as above comment, can event_port_id can be change to source_id? > > >> +/** >> + * Create a dispatcher with the specified id. >> + * >> + * @param id >> + * An application-specified, unique (across all dispatcher >> + * instances) identifier. >> + * >> + * @param event_dev_id >> + * The identifier of the event device from which this dispatcher >> + * will dequeue events. >> + * >> + * @return >> + * - 0: Success >> + * - <0: Error code on failure >> + */ >> +__rte_experimental >> +int >> +rte_dispatcher_create(uint8_t id, uint8_t event_dev_id); > > Following could be used to abstract more dispatcher sources, like > > enum rte_dispatcher_source { > RTE_DISPATCHER_SOURCE_EVENTDEV, // Use rte_event_dequeue() to > pull the packet > RTE_DISPATCHER_SOURCE_ETHDEV, // Use rte_ethdev_rx_burst() to > pull the packet > }; > > struct rte_dispatcher_params { > enum rte_dispatcher_source source; > union { > /* Valid when source == RTE_DISPATCHER_SOURCE_EVENTDEV */ > struct event_source { > uint8_t event_dev_id; > uin8_t event_port_id; > }; > /* Valid when source == RTE_DISPATCHER_SOURCE_ETHDEV*/ > struct ethdev_source { > uint16_t ethdev__dev_id; > uin16_t ethdev_rx_queue_id; > }; > } > }; > > rte_dispatcher_create(uint8_t id, struct rte_dispatcher_params *parms); > > I will stop reviewing at this point. Will review based on direction agree on. Thanks!