From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?UTF-8?Q?Mattias_R=c3=b6nnblom?= Subject: Re: [PATCH v3 0/2] Timer library changes Date: Wed, 19 Dec 2018 08:33:20 +0100 Message-ID: References: <1544205180-31546-1-git-send-email-erik.g.carrillo@intel.com> <1544739996-26011-1-git-send-email-erik.g.carrillo@intel.com> <1740997.qR0t5JnZGs@xps> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: Erik Gabriel Carrillo , rsanford@akamai.com, stephen@networkplumber.org, jerin.jacob@caviumnetworks.com, pbhagavatula@caviumnetworks.com To: Thomas Monjalon , dev@dpdk.org Return-path: Received: from mail.lysator.liu.se (mail.lysator.liu.se [130.236.254.3]) by dpdk.org (Postfix) with ESMTP id AE9EE1B727 for ; Wed, 19 Dec 2018 08:33:25 +0100 (CET) Received: from mail.lysator.liu.se (localhost [127.0.0.1]) by mail.lysator.liu.se (Postfix) with ESMTP id 589BA40037 for ; Wed, 19 Dec 2018 08:33:25 +0100 (CET) In-Reply-To: <1740997.qR0t5JnZGs@xps> Content-Language: en-US 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 2018-12-19 04:35, Thomas Monjalon wrote: > 13/12/2018 23:26, Erik Gabriel Carrillo: >> This patch series modifies the timer library in such a way that >> structures that used to be statically allocated in a process's data >> segment are now allocated in shared memory. As these structures contain >> lists of timers, new APIs are introduced that allow a caller to specify >> the particular structure instance into which a timer should be inserted >> or from which a timer should be removed. This enables primary and >> secondary processes to modify the same timer list, which enables some >> multi-process use cases that were not previously possible; e.g. a >> secondary process can start a timer whose expiration is detected in a >> primary process running a new flavor of timer_manage(). >> >> The original library API is mostly unchanged, though implementations are >> updated to call into newly added functions with a default structure >> instance ID that provides the original behavior. New functions are >> introduced to enable applications to allocate structure instances to >> house timer lists, and to reference them with an identifier when >> starting and stopping timers, and finally, to manage the timer lists >> referenced with an identifier. >> >> My initial performance testing with the "timer_perf_autotest" test shows >> no performance regression or improvement, and inspection of the >> generated optimized code shows that the extra function call gets inlined >> in the functions that now have an extra function call. >> >> Depends on: https://patches.dpdk.org/patch/48417/ >> >> Changes in v3: >> - remove C++ style comment in first patch in series (Stephen) >> >> Changes in v2: >> - split these changes out into their own series >> - version the symbols where the existing ABI was updated, and >> provide alternate implementation with behavior equivalent to original >> behavior. Validated ABI compatibility with validate-abi.sh >> - refactor changes to simplify patches >> >> Erik Gabriel Carrillo (2): >> timer: allow timer management in shared memory >> timer: add function to stop all timers in a list >> >> lib/librte_timer/Makefile | 1 + >> lib/librte_timer/rte_timer.c | 558 ++++++++++++++++++++++++++++++--- >> lib/librte_timer/rte_timer.h | 258 ++++++++++++++- >> lib/librte_timer/rte_timer_version.map | 23 ++ >> 4 files changed, 795 insertions(+), 45 deletions(-) > > It is a lot of changes! > Anyone to review please? > > I can give reviewing the overall aim with the patch set a try: Do we really want DPDK to support more secondary process-based use cases? I would rather see it supporting fewer, with the long term goal of dropping support for secondary processes altogether. DPDK secondary processes are a horrible mess, in my opinion, to put it bluntly.