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 12836A0C4B; Tue, 19 Oct 2021 00:07:43 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C367240142; Tue, 19 Oct 2021 00:07:42 +0200 (CEST) Received: from mail-lf1-f54.google.com (mail-lf1-f54.google.com [209.85.167.54]) by mails.dpdk.org (Postfix) with ESMTP id 469984013F for ; Tue, 19 Oct 2021 00:07:42 +0200 (CEST) Received: by mail-lf1-f54.google.com with SMTP id t9so2926768lfd.1 for ; Mon, 18 Oct 2021 15:07:42 -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=Md0ICNTp7nWQSpKyOf9ss9qx604E3aso6rZJpFj0DtE=; b=pj2iBuEATeEzKwebn1cDIJqgFTE6rdSiJk7CgM2ZQlu1i2N4A+lqpXOegrmUV+wisw xNJ1g/n0/VRdctq4xTMawphskT8OFenBKRP/JPZ2qAHUyiqgXPZFatCsCq6o/beWgGBQ wVj7Efg7TA8z6LwllwoojBERd+Y/lVXwmDKWD7p1abF6OzWROI5CNHJMHwN9S0hL+0pg Oac0AiU0VnbiUKdS6I/3XAvQHpcRcoW0XXpcBZOxfGW2AH2mYHKCd3OjNyUp7nv8l9tV VXmG3ZVOFwdcAmpd+C+rSu8QTJ+Pu3uNvZOsepU6MkwrlRyi8oux5H4N3NJ3b0KaZsw1 ICNA== 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=Md0ICNTp7nWQSpKyOf9ss9qx604E3aso6rZJpFj0DtE=; b=gSuGwZZuu4oVFt8oP6AhY0PZnhlCsOdGtl31X5JfEZZPiZCM4vaMWP5hAvsT2AIJLw al8nml/YgVPLVHktDXZzZvdNAo8z9jar6akCn4fYze5anSaov4af9GhKwU7A1MoHB+Pu Qz2CCUmLz3j3qExAyBKOQ6S9tfs6Uk5eYbJGsj2gFRuiFEfN3yXkod+3KCQpe8zwWI8d CFJ+ybnUow6URVL0/tmSJav7GbjWKamJQX/txDSxwkGgQWnyIXJQV8UPp7EeOgfQODrL Xkqml57D9pJ78zgZzobpJqHicMBeEwnbsl7xPQ+42NbfMeDgkd6yhe5SeSrxqM7jvoI5 FgHQ== X-Gm-Message-State: AOAM531Q1NvcxXmrrkMBs2HyJ8ZQsp5U0obnO5QRns02bWbZ9kQdKm2X eSppXS366fDdytr1JdJnn+s= X-Google-Smtp-Source: ABdhPJzrDhGHIPMA+tPTWXgXvgDO+2UwH+vZQU0KP7Enj6Wo6YF35Zy9vcZaVAslyoOhNCCDE2Jhjg== X-Received: by 2002:a05:6512:31c1:: with SMTP id j1mr2429787lfe.442.1634594861735; Mon, 18 Oct 2021 15:07:41 -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 p15sm1499113lfe.1.2021.10.18.15.07.40 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 18 Oct 2021 15:07:41 -0700 (PDT) Date: Tue, 19 Oct 2021 01:07:40 +0300 From: Dmitry Kozlyuk To: Harman Kalra Cc: , Thomas Monjalon , Ray Kinsella , Message-ID: <20211019010740.232a6d24@sovereign> In-Reply-To: <20211018193707.123559-3-hkalra@marvell.com> References: <20210826145726.102081-1-hkalra@marvell.com> <20211018193707.123559-1-hkalra@marvell.com> <20211018193707.123559-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 v3 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" 2021-10-19 01:07 (UTC+0530), Harman Kalra: [...] > +struct rte_intr_handle *rte_intr_instance_alloc(void) > +{ > + struct rte_intr_handle *intr_handle; > + bool mem_allocator; This name is not very descriptive; what would "mem_allocator is false" mean? How about "is_rte_memory"? > + > + /* Detect if DPDK malloc APIs are ready to be used. */ > + mem_allocator = rte_malloc_is_ready(); > + if (mem_allocator) > + intr_handle = rte_zmalloc(NULL, sizeof(struct rte_intr_handle), > + 0); > + else > + intr_handle = calloc(1, sizeof(struct rte_intr_handle)); > + if (!intr_handle) { > + 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->mem_allocator = mem_allocator; > + > + return intr_handle; > +} > + > +int rte_intr_instance_copy(struct rte_intr_handle *intr_handle, > + const struct rte_intr_handle *src) > +{ > + uint16_t nb_intr; > + > + CHECK_VALID_INTR_HANDLE(intr_handle); > + > + 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; > + > + nb_intr = RTE_MIN(src->nb_intr, intr_handle->nb_intr); Truncating copy is error-prone. It should be either a reallocation (in the future) or an error (now). > + memcpy(intr_handle->efds, src->efds, nb_intr); > + memcpy(intr_handle->elist, src->elist, nb_intr); > + > + return 0; > +fail: > + return -rte_errno; > +} > + > +void rte_intr_instance_free(struct rte_intr_handle *intr_handle) > +{ > + if (intr_handle->mem_allocator) This function should accept NULL and be a no-op in such case. > + rte_free(intr_handle); > + else > + free(intr_handle); > +} [...] > +void *rte_intr_instance_windows_handle_get(struct rte_intr_handle *intr_handle) > +{ > + CHECK_VALID_INTR_HANDLE(intr_handle); > + > + return intr_handle->windows_handle; > +fail: > + return NULL; > +} > + > +int rte_intr_instance_windows_handle_set(struct rte_intr_handle *intr_handle, > + void *windows_handle) > +{ > + CHECK_VALID_INTR_HANDLE(intr_handle); > + > + if (!windows_handle) { > + RTE_LOG(ERR, EAL, "Windows handle should not be NULL\n"); > + rte_errno = EINVAL; > + goto fail; > + } Thanks for adding this API, but please remove the check. It is possible that the API user will pass NULL to reset the state (also NULL is not the only invalid value for a Windows handle). There is no check for Unix FD, neither should be here. > + > + intr_handle->windows_handle = windows_handle; > + > + return 0; > +fail: > + return -rte_errno; > +} [...] > @@ -79,191 +53,20 @@ struct rte_intr_handle { > }; > int fd; /**< interrupt event file descriptor */ > }; > - void *handle; /**< device driver handle (Windows) */ > + void *windows_handle; /**< device driver handle (Windows) */ I guess Windows can be dropped from the comment since it's now in the name. > }; > + bool mem_allocator; > enum rte_intr_handle_type type; /**< handle type */ > uint32_t max_intr; /**< max interrupt requested */ > uint32_t nb_efd; /**< number of available efd(event fd) */ > uint8_t efd_counter_size; /**< size of efd counter, used for vdev */ > + uint16_t nb_intr; > + /**< Max vector count, default RTE_MAX_RXTX_INTR_VEC_ID */ > int efds[RTE_MAX_RXTX_INTR_VEC_ID]; /**< intr vectors/efds mapping */ > struct rte_epoll_event elist[RTE_MAX_RXTX_INTR_VEC_ID]; > - /**< intr vector epoll event */ > + /**< intr vector epoll event */ > + uint16_t vec_list_size; > int *intr_vec; /**< intr vector number array */ > }; > [...] > diff --git a/lib/eal/include/rte_interrupts.h b/lib/eal/include/rte_interrupts.h > index cc3bf45d8c..98edf774af 100644 > --- 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 takes flag as an argument Not anymore. Please update the description. > + * which define from where memory should be allocated i.e. using DPDK memory > + * management library APIs or normal heap allocation. > + * Default memory allocation for event fds and event list array is done which > + * can be realloced later as per the requirement. > + * > + * This function should be called from application or driver, before calling any > + * of the interrupt APIs. > + * > + * @param flags > + * Memory allocation from DPDK allocator or normal allocation > + * > + * @return > + * - On success, address of first interrupt handle. > + * - 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 event fds. event lists > + * and interrupt handle array. It's simpler and more future-proof to just say "interrupt handle resources" instead of enumerating them. > + * > + * @param intr_handle > + * Base address of interrupt handle array. It's not an array anymore. [...] > +/** > + * @internal > + * This API is used to set the event list array index with the given elist "Event list array" sound like an array of lists, while it is really an array of scalar elements. "Event data array"? TBH, I don't know how it's usually named in Unices. > + * instance. > + * > + * @param intr_handle > + * pointer to the interrupt handle. > + * @param index > + * elist array index to be set > + * @param elist > + * event list instance of struct rte_epoll_event > + * > + * @return > + * - On success, zero. > + * - On failure, a negative value. > + */ > +__rte_internal > +int > +rte_intr_elist_index_set(struct rte_intr_handle *intr_handle, int index, > + struct rte_epoll_event elist); > + > +/** > + * @internal > + * Returns the address of elist instance of event list array at a given index. > + * > + * @param intr_handle > + * pointer to the interrupt handle. > + * @param index > + * elist array index to be returned > + * > + * @return > + * - On success, elist > + * - On failure, a negative value. > + */ > +__rte_internal > +struct rte_epoll_event * > +rte_intr_elist_index_get(struct rte_intr_handle *intr_handle, int index); > + > +/** > + * @internal > + * Allocates the memory of interrupt vector list array, with size defining the > + * no of elements required in the array. Typo: "no" -> "number". [...] > + > +/** > + * @internal > + * This API returns the windows handle of the given interrupt instance. Typo: "windows" -> "Windows" here and below. > + * > + * @param intr_handle > + * pointer to the interrupt handle. > + * > + * @return > + * - On success, windows handle. > + * - On failure, NULL. > + */ > +__rte_internal > +void * > +rte_intr_instance_windows_handle_get(struct rte_intr_handle *intr_handle); > + > +/** > + * @internal > + * This API set the windows handle for the given interrupt instance. > + * > + * @param intr_handle > + * pointer to the interrupt handle. > + * @param windows_handle > + * windows handle to be set. > + * > + * @return > + * - On success, zero > + * - On failure, a negative value. > + */ > +__rte_internal > +int > +rte_intr_instance_windows_handle_set(struct rte_intr_handle *intr_handle, > + void *windows_handle); > + > #ifdef __cplusplus > } > #endif > diff --git a/lib/eal/version.map b/lib/eal/version.map > index 38f7de83e1..0ef77c3b40 100644 > --- a/lib/eal/version.map > +++ b/lib/eal/version.map > @@ -109,18 +109,10 @@ DPDK_22 { > rte_hexdump; > rte_hypervisor_get; > rte_hypervisor_get_name; # WINDOWS_NO_EXPORT > - rte_intr_allow_others; > rte_intr_callback_register; > rte_intr_callback_unregister; > - rte_intr_cap_multiple; > - rte_intr_disable; > - rte_intr_dp_is_en; > - rte_intr_efd_disable; > - rte_intr_efd_enable; > rte_intr_enable; > - rte_intr_free_epoll_fd; > - rte_intr_rx_ctl; > - rte_intr_tls_epfd; > + rte_intr_disable; > rte_keepalive_create; # WINDOWS_NO_EXPORT > rte_keepalive_dispatch_pings; # WINDOWS_NO_EXPORT > rte_keepalive_mark_alive; # WINDOWS_NO_EXPORT > @@ -420,6 +412,14 @@ EXPERIMENTAL { > > # added in 21.08 > rte_power_monitor_multi; # WINDOWS_NO_EXPORT > + > + # added in 21.11 > + rte_intr_fd_set; # WINDOWS_NO_EXPORT > + rte_intr_fd_get; # WINDOWS_NO_EXPORT OK, these are not feasible on Windows. > + rte_intr_type_set; # WINDOWS_NO_EXPORT > + rte_intr_type_get; # WINDOWS_NO_EXPORT > + rte_intr_instance_alloc; # WINDOWS_NO_EXPORT > + rte_intr_instance_free; # WINDOWS_NO_EXPORT No, these *are* needed on Windows. > }; > > INTERNAL { > @@ -430,4 +430,33 @@ INTERNAL { > rte_mem_map; > rte_mem_page_size; > rte_mem_unmap; > + rte_intr_cap_multiple; > + rte_intr_dp_is_en; > + rte_intr_efd_disable; > + rte_intr_efd_enable; > + rte_intr_free_epoll_fd; > + rte_intr_rx_ctl; > + rte_intr_allow_others; > + rte_intr_tls_epfd; > + rte_intr_dev_fd_set; # WINDOWS_NO_EXPORT > + rte_intr_dev_fd_get; # WINDOWS_NO_EXPORT OK. > + rte_intr_instance_copy; # WINDOWS_NO_EXPORT > + rte_intr_event_list_update; # WINDOWS_NO_EXPORT > + rte_intr_max_intr_set; # WINDOWS_NO_EXPORT > + rte_intr_max_intr_get; # WINDOWS_NO_EXPORT These are needed on Windows. > + rte_intr_nb_efd_set; # WINDOWS_NO_EXPORT > + rte_intr_nb_efd_get; # WINDOWS_NO_EXPORT > + rte_intr_nb_intr_get; # WINDOWS_NO_EXPORT > + rte_intr_efds_index_set; # WINDOWS_NO_EXPORT > + rte_intr_efds_index_get; # WINDOWS_NO_EXPORT OK. > + rte_intr_elist_index_set; # WINDOWS_NO_EXPORT > + rte_intr_elist_index_get; # WINDOWS_NO_EXPORT These are needed on Windows. > + rte_intr_efd_counter_size_set; # WINDOWS_NO_EXPORT > + rte_intr_efd_counter_size_get; # WINDOWS_NO_EXPORT OK. > + rte_intr_vec_list_alloc; # WINDOWS_NO_EXPORT > + rte_intr_vec_list_index_set; # WINDOWS_NO_EXPORT > + rte_intr_vec_list_index_get; # WINDOWS_NO_EXPORT > + rte_intr_vec_list_free; # WINDOWS_NO_EXPORT These are needed on Windows. > + rte_intr_instance_windows_handle_get; > + rte_intr_instance_windows_handle_set; > };