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 530CDA0A0C; Thu, 15 Apr 2021 09:42:50 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id E964116209A; Thu, 15 Apr 2021 09:42:49 +0200 (CEST) Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by mails.dpdk.org (Postfix) with ESMTP id 88E5E162091 for ; Thu, 15 Apr 2021 09:42:48 +0200 (CEST) IronPort-SDR: avyQ/wUsUnhkd2odoPbTb2cHWPV8c2hAohDIOBn9uW6OlIsvm8ugbodZpKREiZ2TLtxazUTN/W YG9GQGZHnaJA== X-IronPort-AV: E=McAfee;i="6200,9189,9954"; a="194368786" X-IronPort-AV: E=Sophos;i="5.82,223,1613462400"; d="scan'208";a="194368786" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga103.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 15 Apr 2021 00:42:45 -0700 IronPort-SDR: w/MuSG+/nOVVnAXDHwBWfxkd31lQWZ+2smHYPZvooGSyheD71PzfwQm0C8Smll/+Hnu7mwg1J6 M4Jzlip4zHLw== X-IronPort-AV: E=Sophos;i="5.82,223,1613462400"; d="scan'208";a="425079520" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.213.204.163]) ([10.213.204.163]) by orsmga008-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 15 Apr 2021 00:42:42 -0700 To: Suanming Mou , Ori Kam , Andrew Rybchenko , NBU-Contact-Thomas Monjalon Cc: "dev@dpdk.org" , Stephen Hemminger , Matan Azrad , Shahaf Shuler , Ajit Khaparde , Somnath Kotur References: <20210315192722.35490-1-stephen@networkplumber.org> <20210315192722.35490-2-stephen@networkplumber.org> <7106da73-95a1-30ae-f949-87ecca05b24d@intel.com> From: Ferruh Yigit X-User: ferruhy Message-ID: <14c8924a-24cc-b857-2a5c-260b0fcc02ef@intel.com> Date: Thu, 15 Apr 2021 08:42:39 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH 1/2] ethdev: make flow API primary/secondary process safe 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 4/15/2021 3:55 AM, Suanming Mou wrote: > Hi, > >> -----Original Message----- >> From: Ferruh Yigit >> Sent: Wednesday, April 14, 2021 9:07 PM >> To: Suanming Mou ; Ori Kam ; >> Andrew Rybchenko ; NBU-Contact-Thomas >> Monjalon >> Cc: dev@dpdk.org; Stephen Hemminger >> Subject: Re: [dpdk-dev] [PATCH 1/2] ethdev: make flow API primary/secondary >> process safe >> >> On 3/16/2021 11:48 PM, Suanming Mou wrote: >>> Hi Stephen, >>> >>>> -----Original Message----- >>>> From: Stephen Hemminger >>>> Sent: Tuesday, March 16, 2021 3:27 AM >>>> To: dev@dpdk.org >>>> Cc: Stephen Hemminger ; Suanming Mou >>>> >>>> Subject: [PATCH 1/2] ethdev: make flow API primary/secondary process >>>> safe >>>> >>>> Posix mutex are not by default safe for protecting for usage from >>>> multiple processes. The flow ops mutex could be used by both primary >>>> and secondary processes. >>> >>> Process safe is something more widely scope. I assume it should be another >> feature but not a bugfix for thread-safe? >>> And the fag RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE we have added is just >> thread safe. >>> >> >> Hi Suanming, >> >> I think 'RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE' flag and what this patch >> address are different issues. >> >> 'RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE' is to add/remove synchronization >> support for flow APIs, that is for thread safety as flag name suggests. >> >> This patch is to solve the problem for multi process, where commit log describes >> as posix mutex is not safe for multiple process. > > So for PMDs which not set the RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE capability bit, they will have the process level protection in multi-process. > For PMDs which set the RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE capability bit, this change does not help with these PMDs. If the PMD with RTE_ETH_DEV_FLOW_OPS_THREAD_SAFE capability bit internally does not support multi-process, they may still suffer crash etc. Correct > (If I understand correctly, mlx PMD level now should support multi-process, but better to have the confirmation from maintainers with much deeper level). > I assume this patch solves the posix mutex for multi-process only, hard to say the flow API primary/secondary process safe after that patch. > I am also not quite sure how PMDs that doesn't require mutex at all, (mlx5, bnxt, sfc) behave on multi process. Is calling flow APIs from both primary/secondary safe? >> >> >> Stephen, >> Are you aware of any downside setting 'PTHREAD_PROCESS_SHARED' attribute >> to the >> mutex? Any possible performance implications? >> >> Ori, >> Since mlx is heavily using the flow API, is it possible to test this patch? If >> there is no negative impact, I think we can get this patch, what do you think? >> >>>> >>>> Bugzilla ID: 662 >>>> Signed-off-by: Stephen Hemminger >>>> Fixes: 80d1a9aff7f6 ("ethdev: make flow API thread safe") >>>> Cc: suanmingm@nvidia.com >>>> --- >>>> lib/librte_ethdev/rte_ethdev.c | 6 +++++- >>>> 1 file changed, 5 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c >> index >>>> 6f514c388b4e..d1024df408a5 100644 >>>> --- a/lib/librte_ethdev/rte_ethdev.c >>>> +++ b/lib/librte_ethdev/rte_ethdev.c >>>> @@ -470,6 +470,7 @@ rte_eth_dev_allocate(const char *name) { >>>> uint16_t port_id; >>>> struct rte_eth_dev *eth_dev = NULL; >>>> + pthread_mutexattr_t attr; >>>> size_t name_len; >>>> >>>> name_len = strnlen(name, RTE_ETH_NAME_MAX_LEN); @@ -506,7 >>>> +507,10 @@ rte_eth_dev_allocate(const char *name) >>>> strlcpy(eth_dev->data->name, name, sizeof(eth_dev->data->name)); >>>> eth_dev->data->port_id = port_id; >>>> eth_dev->data->mtu = RTE_ETHER_MTU; >>>> - pthread_mutex_init(ð_dev->data->flow_ops_mutex, NULL); >>>> + >>>> + pthread_mutexattr_init(&attr); >>>> + pthread_mutexattr_setpshared(&attr, PTHREAD_PROCESS_SHARED); >>>> >>>> + pthread_mutex_init(ð_dev->data->flow_ops_mutex, &attr); >>>> >>>> unlock: >>>> rte_spinlock_unlock(ð_dev_shared_data->ownership_lock); >>>> -- >>>> 2.30.2 >>> >