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 9B18142605; Tue, 19 Sep 2023 12:59:14 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 1FD9240278; Tue, 19 Sep 2023 12:59:14 +0200 (CEST) Received: from mail-vs1-f46.google.com (mail-vs1-f46.google.com [209.85.217.46]) by mails.dpdk.org (Postfix) with ESMTP id 0534D40276; Tue, 19 Sep 2023 12:59:12 +0200 (CEST) Received: by mail-vs1-f46.google.com with SMTP id ada2fe7eead31-4527d7f7305so752826137.1; Tue, 19 Sep 2023 03:59:12 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1695121152; x=1695725952; 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=LTNkJNCJVkeuulBf/u3NMXNluURcxxmAYcLP8jsUiuA=; b=TXrLy8d+YwspSiSe5kM7aagZP6hcgYH7Ae0El9inbdYcAJyX1HArHEPyaEsZyZHy3L U8nJyrkmoMhh1PToXnGY2MrLRXdaE4Om1A74L7l5bNCqGlVR5+UejWEwYlTk+TEwvjaH NG1V6dGBKhIL4uKsL3QdaGhfcHKckz9TgPu5KeNDlEmmDlpa3fjrCAccKrZ057kRh6bG Sw4IVaGDUAkTkLX5gXVFRuQdSrId2oH/p7qkUqSkU4HUTHLNhd96Cn5VLsHr5kAgvUdX M5YtwHMEc4E6teYniliHDm3qYBp905LjLq11D2MlLJRoYtA0XhF20NyDmC6zFHQ3ynw6 dzLw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1695121152; x=1695725952; 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=LTNkJNCJVkeuulBf/u3NMXNluURcxxmAYcLP8jsUiuA=; b=fcLgqqL5I0Q2zSAEB9AjjABv+MJ789Bbz09z/z0V4uipmBvkHfl2B9dKLEfcBqapi+ bTu6ZUX3kaNHjXVlGnfdCNYAlD75EIp4MiGXO90AIdPc0QfMBe5lPqZn79fTTOoPxi6i 8NEvKbJKGSEwhk1ge6SQoyCwN2GjJMR10WRHp6Jw94ivjP31Syw/dXb1tk7UTaBh+3yz mv4cu0u1ZcnJgsQyfGFi53Rbx1RUxAIqW8xn7BjSqqbCbiqgk98ql0KNsP4HtHPMh4r9 joQUarcDqEfmEuMJnqEU8D5abF6RMK5sLTOuwc+keP3cTEhjsodbMEZx1QQubB0pcJuV Fmiw== X-Gm-Message-State: AOJu0Yw8rfFDhuuyZYpj5p4ipcWZ0vimDYXWhs2wgsg4fiNUv8bsSln2 FltZt3zRZOvJaSdVIswC+msKrS06D4krOw18vho= X-Google-Smtp-Source: AGHT+IFa6OrQ6dbo2ecS0yyW7fEsRuPIx1Y6RdXSx/kNaHRIz6fT5WXcLjZVIsKHXfiOzDq9S9VbHjLGe2E7Z/cypJI= X-Received: by 2002:a05:6102:e57:b0:451:73bb:9fa0 with SMTP id p23-20020a0561020e5700b0045173bb9fa0mr9808422vst.34.1695121152177; Tue, 19 Sep 2023 03:59:12 -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: <20230904130313.327809-2-mattias.ronnblom@ericsson.com> From: Jerin Jacob Date: Tue, 19 Sep 2023 16:28:46 +0530 Message-ID: Subject: Re: [PATCH v3 1/3] lib: introduce dispatcher library To: =?UTF-8?Q?Mattias_R=C3=B6nnblom?= Cc: dev@dpdk.org, Jerin Jacob , techboard@dpdk.org, harry.van.haaren@intel.com, hofors@lysator.liu.se, 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 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. 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. High level cosmetic comment ---------------------------------------------------- 1)Missing doxygen connection- See doc/api/doxy-api-index.md 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. > diff --git a/lib/dispatcher/rte_dispatcher.h b/lib/dispatcher/rte_dispatc= her.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. > +/** > + * @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 =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 about) [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 =3D=3D RTE_DISPATCHER_SOURCE_EVENTD= EV */ struct event_source { uint8_t event_dev_id; uin8_t event_port_id; }; /* Valid when source =3D=3D 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 o= n.