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 9EB23A0548; Mon, 10 May 2021 10:18:06 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 1586840140; Mon, 10 May 2021 10:18:06 +0200 (CEST) Received: from mail-lf1-f43.google.com (mail-lf1-f43.google.com [209.85.167.43]) by mails.dpdk.org (Postfix) with ESMTP id 4A3AD4003E for ; Mon, 10 May 2021 10:18:05 +0200 (CEST) Received: by mail-lf1-f43.google.com with SMTP id x19so22100534lfa.2 for ; Mon, 10 May 2021 01:18:05 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=semihalf-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=7jRdUL6DcF06JrKF7/hdGiT7ZhX9RfQBRp2JVPlkSic=; b=GQ2rYVR78jUVnYpUrzUgMC4DW+TjYNANXR3NZtEr6qIerAOl/guVkf6QY8uOCSCe19 javf7/huTgF3CVdEqpetAd9UWrzyQiLVnUsx8T3Oa5JHz9rGbb5bkFkqjY6k3k8WhQu1 ZldY8KiWw4CV3+mx89ZYg6AamTdE1bslSpAfX9OHxF8GYzLEspYOIjkwEAhpyaJfxl7D vfta+MrGMOa1yEcK93FttnAitqSyay2CgKiOyC8hnXAMS12xA88KUVJZQ0iwENIi0MbI Vs/3fcKwbwvlV0+OsAukbR71Sck/SeSL8SSC3t6s0eVf6xUazqp+2Sv2V+R8pFpxq9Hc fAJQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=7jRdUL6DcF06JrKF7/hdGiT7ZhX9RfQBRp2JVPlkSic=; b=Br2f6F/Zb4ilrmYpKBkUWbB29m0PgLJ00Wf7RE8d+YS4zxMliZc+w5kU2v2y7O3niZ zxqKTVNh0Akhx0UGKsXSXxllmq89fW21sZri5u//SPvksCtQ1u/exN4rsnEqaKw/Pk4W qWKYmikTAZ30UMpNOGID8pe0rf3lh+CZFKf0WYTOM+uKVXpj8bSKWOanIGYEo62HBHBe D8LAPbRfurrXCf/6jU/jKcdiBuwbJzALl5iOMUgVVQr39RkNSkho9tvtXuw1MwHysrwv geNpyfHXD4A2tTubvuaOAJSAfmSURuw4OpD8XSfntfqR3xpac74P2GnMtClM7eW0YgZ1 nDPw== X-Gm-Message-State: AOAM531VN/xbkbhL86YkEwV3+1xfXsD5DTTiHZs/dbw7AbBTA3eSAbrs Oyqj7vf8QNX5OjRehKhtdn/WWw== X-Google-Smtp-Source: ABdhPJwGwjhT4ol6h4xPBND8Km2t+qWIQKD9lBv3dedyU67U/Fdc6p9hOzC9dE/TnAs1uJ8CdG68Rw== X-Received: by 2002:a19:f00c:: with SMTP id p12mr16050210lfc.502.1620634684717; Mon, 10 May 2021 01:18:04 -0700 (PDT) Received: from toster (89-73-146-138.dynamic.chello.pl. [89.73.146.138]) by smtp.gmail.com with ESMTPSA id f10sm3141010lja.83.2021.05.10.01.18.03 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 10 May 2021 01:18:04 -0700 (PDT) Date: Mon, 10 May 2021 10:18:02 +0200 From: Stanislaw Kardach To: Ferruh Yigit Cc: Michal Krawczyk , dev@dpdk.org, ndagan@amazon.com, gtzalik@amazon.com, igorch@amazon.com, upstream@semihalf.com, Shay Agroskin Message-ID: <20210510081802.zlawxarmazqfg6rw@toster> References: <20210505073348.6394-1-mk@semihalf.com> <20210506142526.28245-1-mk@semihalf.com> <20210506142526.28245-18-mk@semihalf.com> <0acf6ac4-aa08-6f33-75a4-90c76745c97a@intel.com> <20210507171800.rynllyxtpzv2nfxr@toster> <08b3c076-7bb2-27c7-5972-12f15f262481@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <08b3c076-7bb2-27c7-5972-12f15f262481@intel.com> Subject: Re: [dpdk-dev] [PATCH v3 17/22] net/ena: support SMP for mz alloc counter 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" On Sun, May 09, 2021 at 02:41:18PM +0100, Ferruh Yigit wrote: > On 5/7/2021 6:18 PM, Stanislaw Kardach wrote: > > On Fri, May 07, 2021 at 05:48:50PM +0100, Ferruh Yigit wrote: > >> On 5/6/2021 3:25 PM, Michal Krawczyk wrote: > >>> From: Stanislaw Kardach > >>> > >>> Introduce a memory area for ENA driver shared between all the processes > >>> of a same prefix (memzone backed). > >>> Move the memzone allocation counter for ENA_MEM_ALLOC_COHERENT there so > >>> that all processes may utilize it. > >> > >> Device private data is already shared between primary/secondary processes, why > >> not using it, it is already there. > > Please correct me if I'm wrong, the dev->data->dev_private is a > > per-device space, whereas the memzone here is used as a shared memory > > between all ena devices. > > Yes it is per-device, so instead of keeping the shared are pointer as global > variable, it is possible to keep this pointer in the device private area, and > initialize per device. This way shared area can be reached by both primary and > secondary applications without additional check. > I think this works better to store device related shared data, like RSS key. > > But I am not sure about 'ena_alloc_cnt', I am not clear what it is, it looks > like intention is to make it accesible from all devices and all processes. Come to think of it, I think we can completely eliminate this memzone. ena_alloc_cnt is used for unique memzone name generation but same could be achieved with port_id + per_device_counter. Thanks for the suggestion, we'll re-think this patch. > > > Btw, How this shared memory freed? Since I think we can get by without it, this question becomes academic, though interesting. Short answer is can't be freed. The reason for that is in multi-process mode the primary process may die while secondaries continue. It means the daemon thread which handles FD passing will die and hence secondaries cannot perform memory alloc/free. So even using reference count to delay memzone free won't help us if the primary is dead. > > > More precisely the memzone counter used here is required to wrap some of > > the base ena code we are calling and may be called from the context of > > any device. Given that memzone names have to be unique, I need this > > counter to be unique too. > > I believe similar thing is done in mlx5 driver > > (mlx5_init_shared_data()). If there is a better way to register such a > > shared segment that is going to be preserved until all processes > > (primary and secondary) are closed I would be happy to rework this. > >> > >> Next patch sharing RSS key using this shared area, can you device private data > >> so all devices can access it. > > It is somewhat similar case. The default key there is generated once for > > all devices and then used in each of them. > >> > >>> > >>> Signed-off-by: Stanislaw Kardach > >>> Reviewed-by: Michal Krawczyk > >>> Reviewed-by: Igor Chauskin > >>> Reviewed-by: Shay Agroskin > >>> --- > >>> drivers/net/ena/base/ena_plat_dpdk.h | 6 ++-- > >>> drivers/net/ena/ena_ethdev.c | 46 +++++++++++++++++++++++++++- > >>> drivers/net/ena/ena_ethdev.h | 8 +++++ > >>> 3 files changed, 56 insertions(+), 4 deletions(-) > >>> > >>> diff --git a/drivers/net/ena/base/ena_plat_dpdk.h b/drivers/net/ena/base/ena_plat_dpdk.h > >>> index 1d0454bebe..e17970361a 100644 > >>> --- a/drivers/net/ena/base/ena_plat_dpdk.h > >>> +++ b/drivers/net/ena/base/ena_plat_dpdk.h > >>> @@ -209,7 +209,7 @@ typedef struct { > >>> * Each rte_memzone should have unique name. > >>> * To satisfy it, count number of allocations and add it to name. > >>> */ > >>> -extern rte_atomic64_t ena_alloc_cnt; > >>> +extern rte_atomic64_t *ena_alloc_cnt; > >>> > >>> #define ENA_MEM_ALLOC_COHERENT_ALIGNED( \ > >>> dmadev, size, virt, phys, mem_handle, alignment) \ > >>> @@ -219,7 +219,7 @@ extern rte_atomic64_t ena_alloc_cnt; > >>> if (size > 0) { \ > >>> char z_name[RTE_MEMZONE_NAMESIZE]; \ > >>> snprintf(z_name, sizeof(z_name), "ena_alloc_%"PRIi64"",\ > >>> - rte_atomic64_add_return(&ena_alloc_cnt, 1)); \ > >>> + rte_atomic64_add_return(ena_alloc_cnt, 1)); \ > >>> mz = rte_memzone_reserve_aligned(z_name, size, \ > >>> SOCKET_ID_ANY, RTE_MEMZONE_IOVA_CONTIG,\ > >>> alignment); \ > >>> @@ -249,7 +249,7 @@ extern rte_atomic64_t ena_alloc_cnt; > >>> if (size > 0) { \ > >>> char z_name[RTE_MEMZONE_NAMESIZE]; \ > >>> snprintf(z_name, sizeof(z_name), "ena_alloc_%"PRIi64"",\ > >>> - rte_atomic64_add_return(&ena_alloc_cnt, 1)); \ > >>> + rte_atomic64_add_return(ena_alloc_cnt, 1)); \ > >>> mz = rte_memzone_reserve_aligned(z_name, size, \ > >>> node, RTE_MEMZONE_IOVA_CONTIG, alignment); \ > >>> mem_handle = mz; \ > >>> diff --git a/drivers/net/ena/ena_ethdev.c b/drivers/net/ena/ena_ethdev.c > >>> index 5d107775f4..0780e2fee2 100644 > >>> --- a/drivers/net/ena/ena_ethdev.c > >>> +++ b/drivers/net/ena/ena_ethdev.c > >>> @@ -83,11 +83,15 @@ struct ena_stats { > >>> /* Device arguments */ > >>> #define ENA_DEVARG_LARGE_LLQ_HDR "large_llq_hdr" > >>> > >>> +#define ENA_MZ_SHARED_DATA "ena_shared_data" > >>> + > >>> /* > >>> * Each rte_memzone should have unique name. > >>> * To satisfy it, count number of allocation and add it to name. > >>> */ > >>> -rte_atomic64_t ena_alloc_cnt; > >>> +rte_atomic64_t *ena_alloc_cnt; > >>> + > >>> +struct ena_shared_data *ena_shared_data; > >>> > >>> static const struct ena_stats ena_stats_global_strings[] = { > >>> ENA_STAT_GLOBAL_ENTRY(wd_expired), > >>> @@ -1752,6 +1756,42 @@ static uint32_t ena_calc_max_io_queue_num(struct ena_com_dev *ena_dev, > >>> return max_num_io_queues; > >>> } > >>> > >>> +static void ena_prepare_shared_data(struct ena_shared_data *shared_data) > >>> +{ > >>> + memset(shared_data, 0, sizeof(*shared_data)); > >>> +} > >>> + > >>> +static int ena_shared_data_init(void) > >>> +{ > >>> + const struct rte_memzone *mz; > >>> + > >>> + if (ena_shared_data != NULL) > >>> + return 0; > >>> + > >>> + if (rte_eal_process_type() == RTE_PROC_PRIMARY) { > >>> + /* Allocate shared memory. */ > >>> + mz = rte_memzone_reserve(ENA_MZ_SHARED_DATA, > >>> + sizeof(*ena_shared_data), > >>> + SOCKET_ID_ANY, 0); > >>> + if (mz == NULL) { > >>> + PMD_INIT_LOG(CRIT, "Cannot allocate ena shared data"); > >>> + return -rte_errno; > >>> + } > >>> + ena_prepare_shared_data(mz->addr); > >>> + } else { > >>> + /* Lookup allocated shared memory. */ > >>> + mz = rte_memzone_lookup(ENA_MZ_SHARED_DATA); > >>> + if (mz == NULL) { > >>> + PMD_INIT_LOG(CRIT, "Cannot attach ena shared data"); > >>> + return -rte_errno; > >>> + } > >>> + } > >>> + ena_shared_data = mz->addr; > >>> + /* Setup ENA_MEM memzone name counter. */ > >>> + ena_alloc_cnt = &ena_shared_data->mz_alloc_cnt; > >>> + return 0; > >>> +} > >>> + > >>> static int eth_ena_dev_init(struct rte_eth_dev *eth_dev) > >>> { > >>> struct ena_calc_queue_size_ctx calc_queue_ctx = { 0 }; > >>> @@ -1773,6 +1813,10 @@ static int eth_ena_dev_init(struct rte_eth_dev *eth_dev) > >>> eth_dev->tx_pkt_burst = ð_ena_xmit_pkts; > >>> eth_dev->tx_pkt_prepare = ð_ena_prep_pkts; > >>> > >>> + rc = ena_shared_data_init(); > >>> + if (rc != 0) > >>> + return rc; > >>> + > >>> if (rte_eal_process_type() != RTE_PROC_PRIMARY) > >>> return 0; > >>> > >>> diff --git a/drivers/net/ena/ena_ethdev.h b/drivers/net/ena/ena_ethdev.h > >>> index ae235897ee..e8858c6118 100644 > >>> --- a/drivers/net/ena/ena_ethdev.h > >>> +++ b/drivers/net/ena/ena_ethdev.h > >>> @@ -207,6 +207,14 @@ struct ena_offloads { > >>> bool rx_csum_supported; > >>> }; > >>> > >>> +/* Holds data shared between all instances of ENA PMD. */ > >>> +struct ena_shared_data { > >>> + /* Each rte_memzone should have unique name. > >>> + * To satisfy it, count number of allocation and add it to name. > >>> + */ > >>> + rte_atomic64_t mz_alloc_cnt; > >>> +}; > >>> + > >>> /* board specific private data structure */ > >>> struct ena_adapter { > >>> /* OS defined structs */ > >>> > >> > > > -- Best Regards, Stanislaw Kardach