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 E4AD3A034F; Thu, 14 Oct 2021 02:58:55 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id A5EC440042; Thu, 14 Oct 2021 02:58:55 +0200 (CEST) Received: from mail-lf1-f41.google.com (mail-lf1-f41.google.com [209.85.167.41]) by mails.dpdk.org (Postfix) with ESMTP id F40E340041 for ; Thu, 14 Oct 2021 02:58:53 +0200 (CEST) Received: by mail-lf1-f41.google.com with SMTP id y26so19558458lfa.11 for ; Wed, 13 Oct 2021 17:58:53 -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=WmXi32rKjnzO7Efc16M7JFmqe5W2DomawWElzxLSKHI=; b=Z3zxrYoZSKjMUfK5i6w7/wWCnsD1TKUDenVK1qMywVtFuro18K9evoslEAC+hrnM7+ NsOxbod0QbLjjA1ImlBrhXAS/4seYagROuDIYnUq51ouamZ+SPHgVFD3VmWdWaw1Q11w p3HY8n7VsQ/ZspYWZCCZ730v4rKXbqhScN+xVvaIig8vpkppD4ReH8+bbpthRiFezauH JL2zeHIn6iwMl5rUAW6e0gFwX0ZuRcwTDiqv1d1qDL/oYsWjhqlbGBpnOs1dirpGdFVf dWMx4q3ltuEUuL60hOafsmO4npZQ1PXyo4t5qrqz5Xu73FWrjRx45f/xWwDvui/SHDzy R/Uw== 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=WmXi32rKjnzO7Efc16M7JFmqe5W2DomawWElzxLSKHI=; b=bKT/optndGEbc6im5RwBJDZJ3XH3dpSL8+kJY0w0dXQdxYnBn+tvE8Nu3lRYDF/Jyi +e+2t6x42abJnNhmsKtcQ+aVWXXkEjcaK6HFvmacqAo/bD6QF2EdICY5sq9aMBhUZlHq IpEFF1FNYGDsSYCtyqE7N6JXZw/nbsJgR2gAsCTWlvp8bgB/10KqPamHl96UN/wmhhyt Nw/y9AfQCaoZbJ3zn0nXwFZ5Z2SnkdHcRPxgvTxLZ6hJAJI+DkEc+gs/uVFdoFBzfus7 EDkMHGnWNjW/Pv5iJa1KPF4mfyo4CQYrAeXa57igIJ09x+cgGcG30PGMLksREWHYo/dY +zwQ== X-Gm-Message-State: AOAM5303FbJkutYs6pol8tkg8a1NTcQgnYJAVGEFWnAJP4fU+iCfikeP fYkjfQDB+iX6bBYp6JZTDr8= X-Google-Smtp-Source: ABdhPJxX1AzuJKqyl34dghJMT7vVcC6QFYWXF/PCIxn/U/GFmMr5QThr9QOF3SLonyk3YcrKthkj3g== X-Received: by 2002:a19:c7c9:: with SMTP id x192mr2239397lff.244.1634173133220; Wed, 13 Oct 2021 17:58:53 -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 w18sm106590ljj.134.2021.10.13.17.58.52 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 13 Oct 2021 17:58:52 -0700 (PDT) Date: Thu, 14 Oct 2021 03:58:52 +0300 From: Dmitry Kozlyuk To: Harman Kalra Cc: , Thomas Monjalon , Ray Kinsella , Message-ID: <20211014035852.7bb70657@sovereign> In-Reply-To: <20211005121502.66964-2-hkalra@marvell.com> References: <20210826145726.102081-1-hkalra@marvell.com> <20211005121502.66964-1-hkalra@marvell.com> <20211005121502.66964-2-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 v2 1/6] 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" 2021-10-05 17:44 (UTC+0530), Harman Kalra: > [...] > +int rte_intr_instance_copy(struct rte_intr_handle *intr_handle, > + const struct rte_intr_handle *src) > +{ > + if (intr_handle == NULL) { > + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n"); > + rte_errno = ENOTSUP; > + goto fail; > + } > + > + if (src == NULL) { > + RTE_LOG(ERR, EAL, "Source interrupt instance unallocated\n"); > + rte_errno = EINVAL; > + goto fail; > + } > + > + intr_handle->fd = src->fd; > + intr_handle->vfio_dev_fd = src->vfio_dev_fd; > + intr_handle->type = src->type; > + intr_handle->max_intr = src->max_intr; > + intr_handle->nb_efd = src->nb_efd; > + intr_handle->efd_counter_size = src->efd_counter_size; > + > + memcpy(intr_handle->efds, src->efds, src->nb_intr); > + memcpy(intr_handle->elist, src->elist, src->nb_intr); Buffer overrun if "intr_handle->nb_intr < src->nb_intr"? > + > + return 0; > +fail: > + return -rte_errno; > +} > + > +int rte_intr_instance_mem_allocator_get( > + const struct rte_intr_handle *intr_handle) > +{ > + if (intr_handle == NULL) { > + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n"); > + return -ENOTSUP; ENOTSUP usually means the operation is valid from API standpoint but not supported by the implementation. EINVAL/EFAULT suits better. > + } > + > + return intr_handle->mem_allocator; > +} What do you think about having an API to retrieve the entire flags instead? > + > +void rte_intr_instance_free(struct rte_intr_handle *intr_handle) > +{ > + if (intr_handle == NULL) { > + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n"); > + rte_errno = ENOTSUP; > + } API are neater when free(NULL) is a no-op. > + > + if (intr_handle->mem_allocator) > + rte_free(intr_handle); > + else > + free(intr_handle); > +} > + > +int rte_intr_fd_set(struct rte_intr_handle *intr_handle, int fd) > +{ > + if (intr_handle == NULL) { > + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n"); > + rte_errno = ENOTSUP; > + goto fail; > + } This piece repeats over and over, how about making it a function or a macro, like in ethdev? > + > + intr_handle->fd = fd; > + > + return 0; > +fail: > + return -rte_errno; > +} > + > +int rte_intr_fd_get(const struct rte_intr_handle *intr_handle) > +{ > + if (intr_handle == NULL) { > + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n"); > + rte_errno = ENOTSUP; > + goto fail; > + } > + > + return intr_handle->fd; > +fail: > + return -1; > +} Please add a similar pair of experimental API for the "handle" member, it is needed for Windows interrupt support I'm working on top of these series (IIUC, API changes should be closed by RC1.) If you will be doing this and don't like "handle" name, it might be like "dev_handle" or "windows_device". > [...] > +int rte_intr_max_intr_set(struct rte_intr_handle *intr_handle, > + int max_intr) > +{ > + if (intr_handle == NULL) { > + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n"); > + rte_errno = ENOTSUP; > + goto fail; > + } > + > + if (max_intr > intr_handle->nb_intr) { > + RTE_LOG(ERR, EAL, "Max_intr=%d greater than RTE_MAX_RXTX_INTR_VEC_ID=%d", The macros is not used in the comparison, neither should the log mention it. > [...] > @@ -420,6 +412,14 @@ EXPERIMENTAL { > > # added in 21.08 > rte_power_monitor_multi; # WINDOWS_NO_EXPORT > + > + # added in 21.11 > + rte_intr_fd_set; > + rte_intr_fd_get; WINDOWS_NO_EXPORT > + rte_intr_type_set; > + rte_intr_type_get; > + rte_intr_instance_alloc; > + rte_intr_instance_free; > }; Do I understand correctly that these exports are needed to allow an application to use DPDK callback facilities for its own interrupt sources? If so, I'd suggest that instead we export a simpler set of functions: 1. Create/free a handle instance with automatic fixed type selection. 2. Trigger an interrupt on the specified handle instance. The flow would be that the application listens on whatever it wants, probably with OS-specific mechanisms, and just notifies the interrupt thread about events to trigger callbacks. Because these APIs are experimental we don't need to change it now, just my thoughts for the future.