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 478C742608; Thu, 21 Sep 2023 19:48:09 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C40EC40698; Thu, 21 Sep 2023 19:48:08 +0200 (CEST) Received: from mail-vs1-f48.google.com (mail-vs1-f48.google.com [209.85.217.48]) by mails.dpdk.org (Postfix) with ESMTP id F163D400D7; Thu, 21 Sep 2023 19:48:07 +0200 (CEST) Received: by mail-vs1-f48.google.com with SMTP id ada2fe7eead31-4527d65354bso614978137.0; Thu, 21 Sep 2023 10:48:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1695318487; x=1695923287; darn=dpdk.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=dCx9U8KE+XFWKoA7WjTCswdJkOEg/jpYW7SjEajXtm8=; b=fUKI8/8TMwL82dE2jKxPfmDrZ1x99OzXF+sLgQYDafPgLazbd/3EtQYJdxKqr0Sz0N j2IO+uDtbRGVIgKyRshv/cFDfHHjVcdTjxJMxt0WSMRbkrqv3YwUo0WRvPX/y2rfCOms fmYWn1GqzIs5jdWfzIoeesO2XTBZyvCbCJlNwSKcnJ+f/GN844P+SsnhLfW8LZp9LHIt TyhTYWbVvMrP7EQ2RGYbhSpwwMfCau/WI4OKspEo6uKvMX6IgFYnrD6A4vbco4OoDLr3 OqyqQVRIgTbaK5krpI+NMYHVD8B3zRej5TJ8t75jjqOPah/IDumMc7n7ZJ6mGo30mzrY 99Sw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1695318487; x=1695923287; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=dCx9U8KE+XFWKoA7WjTCswdJkOEg/jpYW7SjEajXtm8=; b=qfa+hw2VRXkEtCqLC0gnIipTsBGSarA4lYuSe7b9PP/8FTmZkX4/jiBtnICu37q+85 nR3R/sawf27uDTTamwFxkwcwIpB3rHrsKRClnjGfXLqvQqhi3fGlsgBxJKQ3toIBWgJP ENQHYvoBS49PSF+a0UU6L73r4iiDSamHNK84ryNzcgWk5URjTWMWRnXd2KVYP0xGfI1e ux7k/LlDiOD9W53bW5vAhUakuDhtTUyzRROJjusKbJgmCj9qpLtmXx5lJ7tkEQt321oz ebVeMtmeBUQhoUKA/5GdkVtv5vM4posesOJuOY/mB4VAe9SRw3KeRm9RKNVmUTpQ6xv9 EjnA== X-Gm-Message-State: AOJu0Yz0ULNzTCZwEbOCS5nMP2V3qvFIntAJvgE1g5x1XdpIe8z0TY9h 9jybxuRGHqsMpO6m/CVxBasd1FtyqgALXpuyqcM= X-Google-Smtp-Source: AGHT+IHaIsFbdhOiUm7mn9wLI9gwFlJbydU0LE1dYXRW3j1UGXVSpifYc+jI10A9KzrtKyDefDhVbmPHj/U3u1oMDH4= X-Received: by 2002:a67:e952:0:b0:452:61f6:4b53 with SMTP id p18-20020a67e952000000b0045261f64b53mr6414689vso.2.1695318487195; Thu, 21 Sep 2023 10:48:07 -0700 (PDT) MIME-Version: 1.0 References: <20230616074041.159675-2-mattias.ronnblom@ericsson.com> <20230904130313.327809-1-mattias.ronnblom@ericsson.com> <20230904130313.327809-2-mattias.ronnblom@ericsson.com> In-Reply-To: From: Jerin Jacob Date: Thu, 21 Sep 2023 23:17:40 +0530 Message-ID: Subject: Re: [PATCH v3 1/3] lib: introduce dispatcher library To: =?UTF-8?Q?Mattias_R=C3=B6nnblom?= 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 , =?UTF-8?Q?Morten_Br=C3=B8rup?= Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 Thu, Sep 21, 2023 at 10:17=E2=80=AFPM Mattias R=C3=B6nnblom wrote: > > On 2023-09-19 12:58, Jerin Jacob wrote: > > On Mon, Sep 4, 2023 at 6:39=E2=80=AFPM Mattias R=C3=B6nnblom > > 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=C3=B6nnblom > >> 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). The only reason why I thought of this, It is cheap to do as all the logic for comparing match actions, packet/event aggregation and calling the action function is _same_ and better type checking can be added by separate callback for each source. and allow more user to use this library. I don't have a strong opinion of API semantic on this library API other than the placement. Feel free to ignore. > Another option for a traditional app calling rte_eth_rx_burst() directly > is to start using eventdev. :) Yes. Those who can afford extra SW cores to emulate eventdev or has evendev= HW. > > > 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 ser= vice 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 Yes. > > 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. In general, more patches easy review and attract more reviewers. Last library was added to dpdk is lib/mldev. You can see git log lib/mldev/ There operations like _create/free() etc made as separate patches. I leave it up to you and Thomas as this library will be merged though main = tree. No strong opinion. > > > > >> diff --git a/lib/dispatcher/rte_dispatcher.h b/lib/dispatcher/rte_disp= atcher.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_dat= a); > > > > > > 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 =3D X has value > > is Y and offset =3D X has value =3D 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 abo= ut) > > > > 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 OK, as you wish.