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 AD117A0C43; Wed, 20 Oct 2021 18:15:10 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 900024111E; Wed, 20 Oct 2021 18:15:10 +0200 (CEST) Received: from mail-lj1-f172.google.com (mail-lj1-f172.google.com [209.85.208.172]) by mails.dpdk.org (Postfix) with ESMTP id 054B5410E9 for ; Wed, 20 Oct 2021 18:15:09 +0200 (CEST) Received: by mail-lj1-f172.google.com with SMTP id r6so13503036ljg.6 for ; Wed, 20 Oct 2021 09:15:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=Lb6nCX8u6ojHI+0M3s2qiAjftAI5HsGCAMnQtQ3xTTA=; b=E+MsLGwQNiydpPvKK6w7SJ9iZlHxpCXxPMxXEHtgIQ7UtfAaJ8/iU9wEbn6YIDIsoH yhBT4BrUmrkQZf/Epsmh21SmK65G1rc6hnfB6Pi5yo+Ox1EriKFQpmfLBxIsSw49fmW9 x+6y5z/PJIC1LA7PH9lVY3peMqBXb9b+Wlj6ozgCQ84kDOuEcY/3NpXr2Rh7jqU0BTIg UcnggDumppQ5PJZ7/19i6BbFMZ2eoq2q2geW2ZoztzF1+PPyNz1gYhORliX2/kuPTX4i s8gRi7Hgv9Il5lay/KxUKbIHzvsV8aJjq+WdfGee9DVdEMFlUP60p3c5drr8I1H5KYip s1FA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=Lb6nCX8u6ojHI+0M3s2qiAjftAI5HsGCAMnQtQ3xTTA=; b=OI8IU005fD+Q2NGOJULKSFago1EIC0XxROoCRyoxe96T+mp0VfxvfULAoSTfcbtWou MBUs7r7Gtavn9cm2Yo86pIizH9BzLeRy1NCfEiN5ZtT2shitymL+YhE+vWV2l8xensWC eLNOrfD+vf/DfgbtqHMyBvsc1i2VlTMnpVP1FDO7h3Erqcy2e4unSSL3bUqBtIHPfXDV iWh86DCUyu7IdJYogx7m9wdvoO8QQIRqjUgI6oBng6RFT0LEgDZb0TpsTv/Z/Mge1BGi ZAaua9A6AN9lk2vbocPEOHrmniA8GkypeUrzt2Wk6ovWSYqr8c/M+DZ8yAKQCnx9DZno KTzw== X-Gm-Message-State: AOAM532Hot/rfrMwG0hacqN8jaeSOpv0fE8Ei6KbXXAJYFo/cGvnYAMG gSPt/f2olazIPyig//TtJno= X-Google-Smtp-Source: ABdhPJyT2M3Gfo3RqeIzZZRE+otntF2jWsGQhkjafhKbOvVJhJDayXTk1xIfX5ytGBvqkBXFjvlFCw== X-Received: by 2002:a05:651c:b25:: with SMTP id b37mr326853ljr.341.1634746508477; Wed, 20 Oct 2021 09:15:08 -0700 (PDT) Received: from sovereign (broadband-37-110-65-23.ip.moscow.rt.ru. [37.110.65.23]) by smtp.gmail.com with ESMTPSA id c15sm20376ljf.34.2021.10.20.09.15.07 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 20 Oct 2021 09:15:08 -0700 (PDT) Date: Wed, 20 Oct 2021 19:15:07 +0300 From: Dmitry Kozlyuk To: Harman Kalra Cc: , Thomas Monjalon , Ray Kinsella , Message-ID: <20211020191442.6f8953f6@sovereign> In-Reply-To: <20211019183543.132084-3-hkalra@marvell.com> References: <20210826145726.102081-1-hkalra@marvell.com> <20211019183543.132084-1-hkalra@marvell.com> <20211019183543.132084-3-hkalra@marvell.com> X-Mailer: Claws Mail 3.18.0 (GTK+ 2.24.32; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v4 2/7] eal/interrupts: implement get set APIs 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 Sender: "dev" Hello Harman, This patch looks good to me, there are just some tiny comments inline. 2021-10-20 00:05 (UTC+0530), Harman Kalra: > [...] > +/* Macros to check for valid port */ > +#define CHECK_VALID_INTR_HANDLE(intr_handle) do { \ > + if (intr_handle == NULL) { \ > + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n"); \ > + rte_errno = EINVAL; \ > + goto fail; \ > + } \ > +} while (0) In most cases "goto fail" could be "return -rte_errno". How about this (feel free to ignore)? #define CHECK_VALID_INTR_HANDLE_RET(intr_handle) do { \ CHECK_VALID_INTR_HANDLE(intr_handle); \ fail: \ return -rte_errno; \ } while (0) > [...] > +struct rte_intr_handle *rte_intr_instance_alloc(void) > +{ > + struct rte_intr_handle *intr_handle; > + bool is_rte_memory; > + > + /* Detect if DPDK malloc APIs are ready to be used. */ > + is_rte_memory = rte_malloc_is_ready(); > + if (is_rte_memory) > + intr_handle = rte_zmalloc(NULL, sizeof(struct > rte_intr_handle), > + 0); > + else > + intr_handle = calloc(1, sizeof(struct rte_intr_handle)); Nit: sizeof(*intr_handle). > + if (!intr_handle) { intr_handle == NULL > + RTE_LOG(ERR, EAL, "Fail to allocate intr_handle\n"); > + rte_errno = ENOMEM; > + return NULL; > + } > + > + intr_handle->nb_intr = RTE_MAX_RXTX_INTR_VEC_ID; > + intr_handle->is_rte_memory = is_rte_memory; > + > + return intr_handle; > +} > + [...] > + > +void rte_intr_instance_free(struct rte_intr_handle *intr_handle) > +{ > + if (intr_handle) { intr_handle != NULL > + if (intr_handle->is_rte_memory) > + rte_free(intr_handle); > + else > + free(intr_handle); > + } > +} [...] > + > +int rte_intr_elist_index_set(struct rte_intr_handle *intr_handle, > + int index, struct rte_epoll_event elist) > +{ > + CHECK_VALID_INTR_HANDLE(intr_handle); > + > + if (index >= intr_handle->nb_intr) { > + RTE_LOG(ERR, EAL, "Invalid size %d, max limit %d\n", index, > + intr_handle->nb_intr); Which "size"? > + rte_errno = ERANGE; > + goto fail; > + } > + > + intr_handle->elist[index] = elist; > + > + return 0; > +fail: > + return -rte_errno; > +} > + [...] > +int rte_intr_vec_list_index_get(const struct rte_intr_handle *intr_handle, > + int index) > +{ > + CHECK_VALID_INTR_HANDLE(intr_handle); > + > + if (!intr_handle->intr_vec) { > + RTE_LOG(ERR, EAL, "Intr vector list not allocated\n"); > + rte_errno = ENOTSUP; > + goto fail; > + } Can be RTE_ASSERT(), because vec_list_size will be 0 in this case. > + > + if (index > intr_handle->vec_list_size) { > + RTE_LOG(ERR, EAL, "Index %d greater than vec list size %d\n", > + index, intr_handle->vec_list_size); > + rte_errno = ERANGE; > + goto fail; > + } > + > + return intr_handle->intr_vec[index]; > +fail: > + return -rte_errno; > +} > + > +int rte_intr_vec_list_index_set(struct rte_intr_handle *intr_handle, > + int index, int vec) > +{ > + CHECK_VALID_INTR_HANDLE(intr_handle); > + > + if (!intr_handle->intr_vec) { > + RTE_LOG(ERR, EAL, "Intr vector list not allocated\n"); > + rte_errno = ENOTSUP; > + goto fail; > + } Same here. > + > + if (index > intr_handle->vec_list_size) { > + RTE_LOG(ERR, EAL, "Index %d greater than vec list size %d\n", > + index, intr_handle->vec_list_size); > + rte_errno = ERANGE; > + goto fail; > + } > + > + intr_handle->intr_vec[index] = vec; > + > + return 0; > +fail: > + return -rte_errno; > +} > + > +void rte_intr_vec_list_free(struct rte_intr_handle *intr_handle) > +{ > + if (intr_handle) { > + rte_free(intr_handle->intr_vec); > + intr_handle->intr_vec = NULL; intr_handle->vec_list_size = 0; > + } > +} > + [...] > diff --git a/lib/eal/include/rte_interrupts.h b/lib/eal/include/rte_interrupts.h [...] > +/** > + * @warning > + * @b EXPERIMENTAL: this API may change without prior notice > + * > + * It allocates memory for interrupt instance. API auto detects if memory > + * for the instance should be allocated using DPDK memory management library > + * APIs or normal heap allocation, based on if DPDK memory subsystem is > + * initialized and ready to be used. This is too much implementation detail and not very specific from user PoV. Suggestion: After rte_eal_init() has finished, it allocates from DDPK heap, otherwise it allocates from normal heap. In particular, it allocates from the normal heap during initial bus scanning. See also my reply to v3 regarding allocation. > + * > + * Default memory allocation for event fds and epoll event array is done which > + * can be realloced later as per the requirement. BTW, why do this? > + * > + * This function should be called from application or driver, before calling any > + * of the interrupt APIs. > + * > + * @return > + * - On success, address of first interrupt handle. Not "first". > + * - On failure, NULL. > + */ > +__rte_experimental > +struct rte_intr_handle * > +rte_intr_instance_alloc(void); > + > +/** > + * @warning > + * @b EXPERIMENTAL: this API may change without prior notice > + * > + * This API is used to free the memory allocated for interrupt handle resources. > + * > + * @param intr_handle > + * Base address of interrupt handle array. Not "array". > + * > + */ > +__rte_experimental > +void > +rte_intr_instance_free(struct rte_intr_handle *intr_handle); > + > +/** > + * @warning > + * @b EXPERIMENTAL: this API may change without prior notice > + * > + * This API is used to set the fd field of interrupt handle with user provided > + * file descriptor. > + * > + * @param intr_handle > + * pointer to the interrupt handle. > + * @param fd > + * file descriptor value provided by user. > + * > + * @return > + * - On success, zero. > + * - On failure, a negative value. + "and rte_errno is set" here and in other places. [...] > +/** > + * @internal > + * This API is used to populate interrupt handle, with src handler fields. Comma is not needed. > + * > + * @param intr_handle > + * Start address of interrupt handles It's a single handle. > + * @param src > + * Source interrupt handle to be cloned. > + * > + * @return > + * - On success, zero. > + * - On failure, a negative value. > + */ > +__rte_internal > +int > +rte_intr_instance_copy(struct rte_intr_handle *intr_handle, > + const struct rte_intr_handle *src); > + [...] > +/** > + * @internal > + * This API is used to set the event fd counter size field of interrupt handle > + * with user provided efd counter size. > + * > + * @param intr_handle > + * pointer to the interrupt handle. > + * @param efd_counter_size > + * size of efd counter, used for vdev No need to mention vdev. > + * > + * @return > + * - On success, zero. > + * - On failure, a negative value. > + */ > +__rte_internal > +int > +rte_intr_efd_counter_size_set(struct rte_intr_handle *intr_handle, > + uint8_t efd_counter_size); > + [...] > +/** > + * @internal > + * Freeing the memory allocated for interrupt vector list array. "Freeing" -> "Frees" > + * > + * @param intr_handle > + * pointer to the interrupt handle. > + * > + * @return > + * - On success, zero > + * - On failure, a negative value. > + */ > +__rte_internal > +void > +rte_intr_vec_list_free(struct rte_intr_handle *intr_handle); > + > +/** > + * @internal > + * Reallocates the size efds and elist array based on size provided by user. > + * By default efds and elist array are allocated with default size > + * RTE_MAX_RXTX_INTR_VEC_ID on interrupt handle array creation. Later on device > + * probe, device may have capability of more interrupts than > + * RTE_MAX_RXTX_INTR_VEC_ID. Hence using this API, PMDs can reallocate the "Hence" word is unexpected, I think it should be removed. > + * arrays as per the max interrupts capability of device. > + * > + * @param intr_handle > + * pointer to the interrupt handle. > + * @param size > + * efds and elist array size. > + * > + * @return > + * - On success, zero > + * - On failure, a negative value. > + */ > +__rte_internal > +int > +rte_intr_event_list_update(struct rte_intr_handle *intr_handle, int size); > + [...]