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 47EAA42483; Wed, 25 Jan 2023 11:38:47 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 3390042D6F; Wed, 25 Jan 2023 11:38:47 +0100 (CET) Received: from mail-vs1-f54.google.com (mail-vs1-f54.google.com [209.85.217.54]) by mails.dpdk.org (Postfix) with ESMTP id 6C77D42D66 for ; Wed, 25 Jan 2023 11:38:45 +0100 (CET) Received: by mail-vs1-f54.google.com with SMTP id i188so19327034vsi.8 for ; Wed, 25 Jan 2023 02:38:45 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=x0m+Duxyn0MmoRiOzkuZEid6e1bGD4Fx4NA8W1OMDY4=; b=bIaTYoNqhjy0aLtGcb97I5sGt/UwpAlbTR+VfQ3zMnUVHZa2xdPZW29QwDP8XsTt5X +g0MoIAhHltGplLmd6pmfc7hS+3Fj1V4UBWS7Pt2gsp6T+HGSsi32MYunEUIjmG6mf/D z37uIKe19uywaXfselmtrAs+zqBN7LczetnKv6Vy3ATXzTvoFSqgTmeaS0kRh8bRjNi/ f/HpETiEZ+30T1u4iDhjQfn4yiriKXqFXhA9zYOV0YjpXO1+7SOTX0j9ELb9vpFbZ2EN Al6gxbtdAf5lzcoKK32gMJMOByiSvTh1aQyiSBZVGuyT8w8yUB8Tn4whsctB9AWWmNss tGQA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=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=x0m+Duxyn0MmoRiOzkuZEid6e1bGD4Fx4NA8W1OMDY4=; b=Hzo2Gw7a5jhNHNk+pXfQsBgGnjOZLgkjxjVnwBLZ8jbaSLfNIApJ9X9IM/qliEuHGN hufZId5TcDoVmJUY9vzuU32shmi1QHjJkNE+Wzfo7C+UaznhSGALO+aD6h1b31gAT8XO dXleH6U47Atkr38f0nJOCDP8hmuX8NpXOxM94bUE5hAssCcUns7CM2xeXdUt67WCal0+ 9NssX27R1W45mwlNmeJquSrtV11efPU5LCmkppws4JwlPrl5WFMPBI2/RW2wfq59/BY0 sG/w+W8CikptG5di5Kkv1woj/rAuNugfqHZ3ENTdH6y1xYfPyD369OTiGErUOTadMGlu Md/g== X-Gm-Message-State: AFqh2kpPsLvMTRb5pllkm6YhYEEKh+8Dj/k2j+oqBj/n3im2ZtpYLF2D iMOGmzbkDbboIOGLkq59V6PzHoghoW0GdAdEWVk= X-Google-Smtp-Source: AMrXdXtPwFeUjfYVX66nTW5qaE9BEzp/03wCvAi6LhiUpI26nhDYY86LU37zLutlOGTYj59KP5StgF7Cz4+jSapMH2Y= X-Received: by 2002:a05:6102:5124:b0:3d3:c7fb:d602 with SMTP id bm36-20020a056102512400b003d3c7fbd602mr4358977vsb.31.1674643124662; Wed, 25 Jan 2023 02:38:44 -0800 (PST) MIME-Version: 1.0 References: <20230107161852.3708690-1-s.v.naga.harish.k@intel.com> <20230123180458.486189-1-s.v.naga.harish.k@intel.com> In-Reply-To: From: Jerin Jacob Date: Wed, 25 Jan 2023 16:08:18 +0530 Message-ID: Subject: Re: [PATCH v2 1/3] eventdev/eth_rx: add params set/get APIs To: "Naga Harish K, S V" Cc: "jerinj@marvell.com" , "Carrillo, Erik G" , "Gujjar, Abhinandan S" , "dev@dpdk.org" , "Jayatheerthan, Jay" Content-Type: text/plain; charset="UTF-8" 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 Wed, Jan 25, 2023 at 3:22 PM Naga Harish K, S V wrote: > > Hi Jerin, > > > -----Original Message----- > > From: Jerin Jacob > > Sent: Wednesday, January 25, 2023 9:42 AM > > To: Naga Harish K, S V > > Cc: jerinj@marvell.com; Carrillo, Erik G ; Gujjar, > > Abhinandan S ; dev@dpdk.org; > > Jayatheerthan, Jay > > Subject: Re: [PATCH v2 1/3] eventdev/eth_rx: add params set/get APIs > > > > On Tue, Jan 24, 2023 at 6:37 PM Naga Harish K, S V > > wrote: > > > > > > Hi Jerin, > > > > > > > -----Original Message----- > > > > From: Jerin Jacob > > > > Sent: Tuesday, January 24, 2023 10:00 AM > > > > To: Naga Harish K, S V > > > > Cc: jerinj@marvell.com; Carrillo, Erik G > > > > ; Gujjar, Abhinandan S > > > > ; dev@dpdk.org; Jayatheerthan, Jay > > > > > > > > Subject: Re: [PATCH v2 1/3] eventdev/eth_rx: add params set/get APIs > > > > > > > > On Mon, Jan 23, 2023 at 11:35 PM Naga Harish K S V > > > > wrote: > > > > > > > > > > The adapter configuration parameters defined in the ``struct > > > > > rte_event_eth_rx_adapter_runtime_params`` can be configured and > > > > > retrieved using ``rte_event_eth_rx_adapter_runtime_params_set`` > > > > > and ``rte_event_eth_tx_adapter_runtime_params_get`` respectively. > > > > > > > > > > Signed-off-by: Naga Harish K S V > > > > > > > > > diff --git a/doc/guides/prog_guide/event_ethernet_rx_adapter.rst > > > > > b/doc/guides/prog_guide/event_ethernet_rx_adapter.rst > > > > > index 461eca566f..2207d6ffc3 100644 > > > > > --- a/doc/guides/prog_guide/event_ethernet_rx_adapter.rst > > > > > +++ b/doc/guides/prog_guide/event_ethernet_rx_adapter.rst > > > > > @@ -185,6 +185,26 @@ flags for handling received packets, event > > > > > queue identifier, scheduler type, event priority, polling > > > > > frequency of the receive queue and flow identifier in struct > > > > ``rte_event_eth_rx_adapter_queue_conf``. > > > > > > > > > > +Set/Get adapter runtime configuration parameters > > > > > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > > + > > > > > +The runtime configuration parameters of adapter can be set/read > > > > > +using ``rte_event_eth_rx_adapter_runtime_params_set()`` and > > > > > +``rte_event_eth_rx_adapter_runtime_params_get()`` respectively. > > > > > +The parameters that can be set/read are defined in ``struct > > > > rte_event_eth_rx_adapter_runtime_params``. > > > > > > > > Good. > > > > > > > > > + > > > > > +``rte_event_eth_rx_adapter_create()`` or > > > > > +``rte_event_eth_rx_adapter_create_with_params()`` configures the > > > > > +adapter with default value for maximum packets processed per > > > > > +request to > > > > 128. > > > > > +``rte_event_eth_rx_adapter_runtime_params_set()`` function allows > > > > > +to reconfigure maximum number of packets processed by adapter per > > > > > +service request. This is alternative to configuring the maximum > > > > > +packets processed per request by adapter by using > > > > > +``rte_event_eth_rx_adapter_create_ext()`` with parameter > > > > ``rte_event_eth_rx_adapter_conf::max_nb_rx``. > > > > > > > > This paragraph is not needed IMO. As it is specific to a driver, and > > > > we can keep Doxygen comment only. > > > > > > > > > > > > > + > > > > > +``rte_event_eth_rx_adapter_runtime_parmas_get()`` function > > > > > +retrieves the configuration parameters that are defined in > > > > > +``struct > > > > rte_event_eth_rx_adapter_runtime_params``. > > > > > > > > Good. > > > > > > > > > + > > > > > Getting and resetting Adapter queue stats > > > > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > > > > > > > diff --git a/lib/eventdev/rte_event_eth_rx_adapter.c > > > > > b/lib/eventdev/rte_event_eth_rx_adapter.c > > > > > index 34aa87379e..d8f3e750b7 100644 > > > > > --- a/lib/eventdev/rte_event_eth_rx_adapter.c > > > > > +++ b/lib/eventdev/rte_event_eth_rx_adapter.c > > > > > @@ -35,6 +35,8 @@ > > > > > #define MAX_VECTOR_NS 1E9 > > > > > #define MIN_VECTOR_NS 1E5 > > > > > > > > > > +#define RXA_NB_RX_WORK_DEFAULT 128 > > > > > + > > > > > #define ETH_RX_ADAPTER_SERVICE_NAME_LEN 32 > > > > > #define ETH_RX_ADAPTER_MEM_NAME_LEN 32 > > > > > > > > > > @@ -1554,7 +1556,7 @@ rxa_default_conf_cb(uint8_t id, uint8_t > > dev_id, > > > > > } > > > > > > > > > > conf->event_port_id = port_id; > > > > > - conf->max_nb_rx = 128; > > > > > + conf->max_nb_rx = RXA_NB_RX_WORK_DEFAULT; > > > > > if (started) > > > > > ret = rte_event_dev_start(dev_id); > > > > > rx_adapter->default_cb_arg = 1; @@ -3436,6 +3438,90 @@ > > > > > rte_event_eth_rx_adapter_instance_get(uint16_t eth_dev_id, > > > > > return -EINVAL; > > > > > } > > > > > > > > > + > > > > > +int > > > > > +rte_event_eth_rx_adapter_runtime_params_set(uint8_t id, > > > > > + struct rte_event_eth_rx_adapter_runtime_params > > > > > +*params) { > > > > > + struct event_eth_rx_adapter *rxa; > > > > > + int ret; > > > > > + > > > > > + if (params == NULL) > > > > > + return -EINVAL; > > > > > + > > > > > + if (rxa_memzone_lookup()) > > > > > + return -ENOMEM; > > > > > > > > Introduce an adapter callback and move SW adapter related logic > > > > under callback handler. > > > > > > > > > > > Do you mean introducing eventdev PMD callback for HW implementation? > > > > Yes. > > > > Can this be taken care as and when the HW implementation is required? OK. As long as when case INTERNAL PORT it return -ENOSUP now. > > > > There are no adapter callback currently for service based SW > > Implementation. > > > > > > > > + > > > > > + rxa = rxa_id_to_adapter(id); > > > > > + if (rxa == NULL) > > > > > + return -EINVAL; > > > > > + > > > > > + ret = rxa_caps_check(rxa); > > > > > + if (ret) > > > > > + return ret; > > > > > + > > > > > + rte_spinlock_lock(&rxa->rx_lock); > > > > > + rxa->max_nb_rx = params->max_nb_rx; > > > > > + rte_spinlock_unlock(&rxa->rx_lock); > > > > > + > > > > > + return 0; > > > > > +} > > > > > + > > > > > +int > > > > > +rte_event_eth_rx_adapter_runtime_params_get(uint8_t id, > > > > > + struct rte_event_eth_rx_adapter_runtime_params > > > > > +*params) { > > > > > + struct event_eth_rx_adapter *rxa; > > > > > + int ret; > > > > > + > > > > > + if (params == NULL) > > > > > + return -EINVAL; > > > > > > > > > > > > Introduce an adapter callback and move SW adapter related logic > > > > under callback handler. > > > > > > > > > > > > > > Same as above > > > > > > > > + > > > > > + if (rxa_memzone_lookup()) > > > > > + return -ENOMEM; > > > > + > > > > > + rxa = rxa_id_to_adapter(id); > > > > > + if (rxa == NULL) > > > > > + return -EINVAL; > > > > > + > > > > > + ret = rxa_caps_check(rxa); > > > > > + if (ret) > > > > > + return ret; > > > > > + > > > > > + params->max_nb_rx = rxa->max_nb_rx; > > > > > + > > > > > + return 0; > > > > > +} > > > > > + > > > > > +/* RX-adapter telemetry callbacks */ > > > > > #define RXA_ADD_DICT(stats, s) rte_tel_data_add_dict_u64(d, #s, > > > > > stats.s) > > > > > > > > > > static int > > > > > diff --git a/lib/eventdev/rte_event_eth_rx_adapter.h > > > > > b/lib/eventdev/rte_event_eth_rx_adapter.h > > > > > index f4652f40e8..214ffd018c 100644 > > > > > --- a/lib/eventdev/rte_event_eth_rx_adapter.h > > > > > +++ b/lib/eventdev/rte_event_eth_rx_adapter.h > > > > > @@ -39,10 +39,21 @@ > > > > > * - rte_event_eth_rx_adapter_queue_stats_reset() > > > > > * - rte_event_eth_rx_adapter_event_port_get() > > > > > * - rte_event_eth_rx_adapter_instance_get() > > > > > + * - rte_event_eth_rx_adapter_runtime_params_get() > > > > > + * - rte_event_eth_rx_adapter_runtime_params_set() > > > > > * > > > > > * The application creates an ethernet to event adapter using > > > > > * rte_event_eth_rx_adapter_create_ext() or > > > > rte_event_eth_rx_adapter_create() > > > > > * or rte_event_eth_rx_adapter_create_with_params() functions. > > > > > + * > > > > > + * rte_event_eth_rx_adapter_create() or > > > > > + rte_event_eth_adapter_create_with_params() > > > > > + * configures the adapter with default value of maximum packets > > > > > + processed per > > > > > + * iteration to RXA_NB_RX_WORK_DEFAULT(128). > > > > > + * rte_event_eth_rx_adapter_runtime_params_set() allows to > > > > > + re-configure maximum > > > > > + * packets processed per iteration. This is alternative to using > > > > > + * rte_event_eth_rx_adapter_create_ext() with parameter > > > > > + * rte_event_eth_rx_adapter_conf::max_nb_rx > > > > > > > > Move this to Doxygen comment against max_nb_rx > > > > > > > > > + * > > > > > * The adapter needs to know which ethernet rx queues to poll for > > > > > mbufs as > > > > well > > > > > * as event device parameters such as the event queue identifier, > > event > > > > > * priority and scheduling type that the adapter should use when > > > > > constructing @@ -299,6 +310,19 @@ struct > > > > rte_event_eth_rx_adapter_params { > > > > > /**< flag to indicate that event buffer is separate for > > > > > each queue */ }; > > > > > > > > > > +/** > > > > > + * Adapter configuration parameters */ struct > > > > > +rte_event_eth_rx_adapter_runtime_params { > > > > > + uint32_t max_nb_rx; > > > > > + /**< The adapter can return early if it has processed at least > > > > > + * max_nb_rx mbufs. This isn't treated as a requirement; batching > > may > > > > > + * cause the adapter to process more than max_nb_rx mbufs. > > > > > > > > Also tell it is valid only for INTERNAL PORT capablity is set. > > > > > > > > > > Do you mean, it is valid only for INTERNAL PORT capability is 'not' set? > > > > Yes. > > > > > > > > > > + */ > > > > > + uint32_t rsvd[15]; > > > > > + /**< Reserved fields for future use */ > > > > > > > > Introduce rte_event_eth_rx_adapter_runtime_params_init() to make > > > > sure rsvd is zero. > > > > > > > > > > The reserved fields are not used by the adapter or application. Not > > > sure Is it necessary to Introduce a new API to clear reserved fields. > > > > When adapter starts using new fileds(when we add new fieds in future), the > > old applicaiton which is not using > > rte_event_eth_rx_adapter_runtime_params_init() may have junk value and > > then adapter implementation will behave bad. > > > > > > does it mean, the application doesn't re-compile for the new DPDK? Yes. No need recompile if ABI not breaking. > When some of the reserved fields are used in the future, the application also may need to be recompiled along with DPDK right? > As the application also may need to use the newly consumed reserved fields? The problematic case is: Adapter implementation of 23.07(Assuming there is change params) field needs to work with application of 23.03. rte_event_eth_rx_adapter_runtime_params_init() will sove that. > > > > > > > > > +}; > > > > > + > > > > > /** > > > > > * > > > > > * Callback function invoked by the SW adapter before it > > > > > continues @@ > > > > > -377,7 +401,7 @@ int rte_event_eth_rx_adapter_create_ext(uint8_t > > > > > id, > > > > uint8_t dev_id, > > > > > * Create a new ethernet Rx event adapter with the specified > > identifier. > > > > > * This function uses an internal configuration function that creates an > > event > > > > > * port. This default function reconfigures the event device with > > > > > an > > > > > - * additional event port and setups up the event port using the > > > > > port_config > > > > > + * additional event port and setup the event port using the > > > > > + port_config > > > > > * parameter passed into this function. In case the application needs > > more > > > > > * control in configuration of the service, it should use the > > > > > * rte_event_eth_rx_adapter_create_ext() version. > > > > > @@ -743,6 +767,50 @@ > > > > > rte_event_eth_rx_adapter_instance_get(uint16_t > > > > eth_dev_id, > > > > > uint16_t rx_queue_id, > > > > > uint8_t *rxa_inst_id); > > > > >