From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg1-f180.google.com (mail-pg1-f180.google.com [209.85.215.180]) by dpdk.org (Postfix) with ESMTP id AE4865B40 for ; Fri, 7 Dec 2018 18:29:40 +0100 (CET) Received: by mail-pg1-f180.google.com with SMTP id 70so2005541pgh.8 for ; Fri, 07 Dec 2018 09:29:40 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=networkplumber-org.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=J/WPGMbb2S/K03/bQY9pJl4JNt1VaruWKFTpZF+FE8Q=; b=tcDznBjOIdAIYr5siL/iQGqebzVU2iQCZA6QTpcW2mvrMnPIPdIn0ffJDZyHcBncMy ugeNOQ6v5MFUQgLBHlhU3k0pDPh4NtriFTARqjRZDYkjRdMV767H6ZhaNdWPXSB9/I9j YIOV5jXldhkdWVw4zsjZ63rp0NlPOnkp0CtzKh+aNRAJNK8VcbmqtyLee+ZbYqwFoMLN mFoYzOZPfE+zquxAmrI5JTcBFw+nQS/Tf8OWSzXIni1EaKbHhbpLb1PcUV1X5JOgra0P 6rlPGzGdK+8vcDf/NS7AOLM+zzG6YV5tMmAQ9xgnge2JLKekWUL/wedYJ8y4xbllk0Q/ 6Sjg== 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:in-reply-to :references:mime-version:content-transfer-encoding; bh=J/WPGMbb2S/K03/bQY9pJl4JNt1VaruWKFTpZF+FE8Q=; b=OolMEChl8yOENmsMtOsKrohxzyhQZt6ToMSbMKIDfEqTxDWGCo3zYwLcexwVI1vPtn 1dU16qNchpsnuv+6PxzbX7BjNIkeah34r8RUIeWyC1JxFvJy3IoJEUhKIheQQQy1/T74 8XTvwodr2Gut111aJpD8vM30lZCDlho9IvWiLXk7/Df6U9YGsPam1b39jZq8O7LaAUOc sLDG2BF13M5STMy8Oo3SXHXFqbZnTp/2n838XmDZb4hEpM6E5psCmc1qvf60iFEBZNB3 YHEdcqgmlifGpD8lUW6NTlEVIAxpKDbNUmP66A2T9CdXe96iXfc8TFjnbD7UfQtCTyHm zp9A== X-Gm-Message-State: AA+aEWbmOJkb8u1h0h1sJt+w++vz5kemepjJ+ZaXPunRK03maIO7PWnI I1P3f3bopeVeatn7XCFRCXKrxA== X-Google-Smtp-Source: AFSGD/VcQeUPB6tT0vCFEkHbVEThpX6ti5FLK+vnQS0TKnhx246ugvwXcwFdJPbXvI4zEuVy6VIn1g== X-Received: by 2002:a63:ee0e:: with SMTP id e14mr2692562pgi.8.1544203779495; Fri, 07 Dec 2018 09:29:39 -0800 (PST) Received: from xeon-e3 (204-195-22-127.wavecable.com. [204.195.22.127]) by smtp.gmail.com with ESMTPSA id a90sm7987021pfj.109.2018.12.07.09.29.39 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Fri, 07 Dec 2018 09:29:39 -0800 (PST) Date: Fri, 7 Dec 2018 09:29:36 -0800 From: Stephen Hemminger To: Honnappa Nagarahalli Cc: "Ananyev, Konstantin" , "dev@dpdk.org" , nd , Dharmik Thakkar , Malvika Gupta , "Gavin Hu (Arm Technology China)" Message-ID: <20181207092936.17bf2887@xeon-e3> In-Reply-To: References: <20181122033055.3431-1-honnappa.nagarahalli@arm.com> <20181122033055.3431-3-honnappa.nagarahalli@arm.com> <2601191342CEEE43887BDE71AB977258010CEBBBA9@IRSMSX106.ger.corp.intel.com> <2601191342CEEE43887BDE71AB977258010CEBD5D4@IRSMSX106.ger.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [RFC 2/3] tqs: add thread quiescent state library X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 07 Dec 2018 17:29:40 -0000 On Fri, 7 Dec 2018 07:27:16 +0000 Honnappa Nagarahalli wrote: > > > > > > > > + > > > > > +/* Add a reader thread, running on an lcore, to the list of > > > > > +threads > > > > > + * reporting their quiescent state on a TQS variable. > > > > > + */ > > > > > +int __rte_experimental > > > > > +rte_tqs_register_lcore(struct rte_tqs *v, unsigned int lcore_id) { > > > > > + TQS_RETURN_IF_TRUE((v == NULL || lcore_id >= > > > > RTE_TQS_MAX_LCORE), > > > > > + -EINVAL); > > > > > > > > It is not very good practice to make function return different > > > > values and behave in a different way in debug/non-debug mode. > > > > I'd say that for slow-path (functions in .c) it is always good to > > > > check input parameters. > > > > For fast-path (functions in .h) we sometimes skip such checking, but > > > > debug mode can probably use RTE_ASSERT() or so. > > > Makes sense, I will change this in the next version. > > > > > > > > > > > > > > > lcore_id >= RTE_TQS_MAX_LCORE > > > > > > > > Is this limitation really necessary? > > > I added this limitation because currently DPDK application cannot take > > > a mask more than 64bit wide. Otherwise, this should be as big as > > RTE_MAX_LCORE. > > > I see that in the case of '-lcores' option, the number of lcores can > > > be more than the number of PEs. In this case, we still need a MAX limit (but > > can be bigger than 64). > > > > > > > First it means that only lcores can use that API (at least data-path > > > > part), second even today many machines have more than 64 cores. > > > > I think you can easily avoid such limitation, if instead of > > > > requiring lcore_id as input parameter, you'll just make it return index of > > next available entry in w[]. > > > > Then tqs_update() can take that index as input parameter. > > > I had thought about a similar approach based on IDs. I was concerned > > > that ID will be one more thing to manage for the application. But, I see the > > limitations of the current approach now. I will change it to allocation based. > > This will support even non-EAL pthreads as well. > > > > Yes, with such approach non-lcore threads will be able to use it also. > > > I realized that rte_tqs_register_lcore/ rte_tqs_unregister_lcore need to be efficient as they can be called from the worker's packet processing loop (rte_event_dequeue_burst allows blocking. So, the worker thread needs to call rte_tqs_unregister_lcore before calling rte_event_dequeue_burst and rte_tqs_register_lcore before starting packet processing). Allocating the thread ID in these functions will make them more complex. > > I suggest that we change the argument 'lcore_id' to 'thread_id'. The application could use 'lcore_id' as 'thread_id' if threads are mapped to physical cores 1:1. > > If the threads are not mapped 1:1 to physical cores, the threads need to use a thread_id in the range of 0 - RTE_TQS_MAX_THREADS. I do not see that DPDK has a thread_id concept. For TQS, the thread IDs are global (i.e. not per TQS variable). I could provide APIs to do the thread ID allocation, but I think the thread ID allocation should not be part of this library. Such thread ID might be useful for other libraries. > >