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 D059DA0C43; Fri, 22 Oct 2021 09:19:27 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 9A86A4069D; Fri, 22 Oct 2021 09:19:27 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id C236D4068C for ; Fri, 22 Oct 2021 09:19:26 +0200 (CEST) Received: from [192.168.38.17] (aros.oktetlabs.ru [192.168.38.17]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id 5735A7F5B3; Fri, 22 Oct 2021 10:19:26 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 5735A7F5B3 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1634887166; bh=QZIuEYP26FwONgyfuF+B8puFm8FIxpUmIR8GUA7Ts6c=; h=Subject:To:Cc:References:From:Date:In-Reply-To; b=xFacjbnj1kWOL6J/8halJHc3W5xSGIMxC3JrtlSIQfqGlrm+IIGuKGHGfFERygpex vjXi5NedCU9aBxMVDbvIXWYWixevUMMi/4VwL4CIX+UnetPIA2usG8K4SEUNTUc180 SxSNMZdigWZxCa3x+EbiBlHCZXsyXEa5ZwXd//Bw= To: David Marchand Cc: dev , Viacheslav Galaktionov , Andy Moreton References: <20211021070355.3547582-1-andrew.rybchenko@oktetlabs.ru> From: Andrew Rybchenko Organization: OKTET Labs Message-ID: <28a57b71-e463-4ac7-50c9-0019bb41cc02@oktetlabs.ru> Date: Fri, 22 Oct 2021 10:19:26 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.14.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH] net/sfc: allow control threads for counter queue polling 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 10/21/21 11:28 PM, David Marchand wrote: > On Thu, Oct 21, 2021 at 9:04 AM Andrew Rybchenko > wrote: >> >> From: Viacheslav Galaktionov >> >> MAE counters can be polled from a control thread if no service core is >> allocated for this. >> >> Signed-off-by: Viacheslav Galaktionov >> Signed-off-by: Andrew Rybchenko >> Reviewed-by: Andy Moreton >> --- >> The problem to require service cores for HW offload was raised by >> David on review in 21.08 release cycle. > > Thanks for following up! > > >> >> doc/guides/rel_notes/release_21_11.rst | 1 + >> drivers/net/sfc/sfc_mae.h | 26 +++++- >> drivers/net/sfc/sfc_mae_counter.c | 120 ++++++++++++++++++++----- >> 3 files changed, 123 insertions(+), 24 deletions(-) >> >> diff --git a/doc/guides/rel_notes/release_21_11.rst b/doc/guides/rel_notes/release_21_11.rst >> index 041383ee2a..9517e0fb0a 100644 >> --- a/doc/guides/rel_notes/release_21_11.rst >> +++ b/doc/guides/rel_notes/release_21_11.rst >> @@ -158,6 +158,7 @@ New Features >> >> * Added port representors support on SN1000 SmartNICs >> * Added flow API transfer proxy support >> + * Added support for flow counters without service cores >> >> * **Updated Marvell cnxk crypto PMD.** >> >> diff --git a/drivers/net/sfc/sfc_mae.h b/drivers/net/sfc/sfc_mae.h >> index 23dcf1e482..45a2fdc3bb 100644 >> --- a/drivers/net/sfc/sfc_mae.h >> +++ b/drivers/net/sfc/sfc_mae.h >> @@ -127,6 +127,13 @@ struct sfc_mae_counters { >> unsigned int n_mae_counters; >> }; >> >> +/** Options for MAE counter polling mode */ >> +enum sfc_mae_counter_polling_mode { >> + SFC_MAE_COUNTER_POLLING_OFF = 0, >> + SFC_MAE_COUNTER_POLLING_SERVICE, >> + SFC_MAE_COUNTER_POLLING_THREAD, >> +}; >> + >> struct sfc_mae_counter_registry { >> /* Common counter information */ >> /** Counters collection */ >> @@ -143,10 +150,21 @@ struct sfc_mae_counter_registry { >> bool use_credits; >> >> /* Information used by configuration routines */ >> - /** Counter service core ID */ >> - uint32_t service_core_id; >> - /** Counter service ID */ >> - uint32_t service_id; >> + enum sfc_mae_counter_polling_mode polling_mode; >> + union { >> + struct { >> + /** Counter service core ID */ >> + uint32_t core_id; >> + /** Counter service ID */ >> + uint32_t id; >> + } service; >> + struct { >> + /** Counter thread ID */ >> + pthread_t id; >> + /** The thread should keep running */ >> + volatile bool run; > > volatile is probably unneeded. Yes, volatile is definitely unnecessary here. Will remove in v2. >> + } thread; >> + } polling; >> }; >> >> /** >> diff --git a/drivers/net/sfc/sfc_mae_counter.c b/drivers/net/sfc/sfc_mae_counter.c >> index 418caffe59..5f2aea1bf4 100644 >> --- a/drivers/net/sfc/sfc_mae_counter.c >> +++ b/drivers/net/sfc/sfc_mae_counter.c >> @@ -45,9 +45,6 @@ sfc_mae_counter_rxq_required(struct sfc_adapter *sa) >> if (encp->enc_mae_supported == B_FALSE) >> return false; >> >> - if (sfc_mae_counter_get_service_lcore(sa) == RTE_MAX_LCORE) >> - return false; >> - >> return true; >> } >> >> @@ -402,6 +399,23 @@ sfc_mae_counter_routine(void *arg) >> return 0; >> } >> >> +static void * >> +sfc_mae_counter_thread(void *data) >> +{ >> + struct sfc_adapter *sa = data; >> + struct sfc_mae_counter_registry *counter_registry = >> + &sa->mae.counter_registry; >> + >> + /* >> + * Check run condition without atomic since it is not a problem >> + * if we run a bit more before we notice stop request >> + */ > > I find it clearer when we have clear pairs of atomic acquire > load/release store (maybe because I feel like I understand something > :-)). > So it may not be a problem, but is there a reason to avoid this > (acquire) atomic load? Atomic in a busy polling loop could affect overall system performance. That's why we avoid it here. However, maybe better solution is to avoid busy polling - just sleep a bit if previous poll is empty. Will address it in v2 in one or another way. > Otherwise, patch lgtm. > Thanks. Many thanks for review, Andrew.