From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg1-f174.google.com (mail-pg1-f174.google.com [209.85.215.174]) by dpdk.org (Postfix) with ESMTP id 289615F17 for ; Fri, 7 Dec 2018 19:10:14 +0100 (CET) Received: by mail-pg1-f174.google.com with SMTP id z10so2052165pgp.7 for ; Fri, 07 Dec 2018 10:10:14 -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=+UPRTkC19Vn7vLSddbivXVgWM2L0V/UYiFOje2tTh0I=; b=pGAHg8UrDx1MqdqlfG2PaPQI/MZagsu+NF34T2T7kDrEUOP8imsL6drC3gsWQCrJKT iTRxsP+E/IoIFi6OQ8y5JM25KWYUqSoCeWDygYHCxpSnUElBw9jq/0fStw4ZFtOKSoDB DDBGbPj4QFdyb72K84AEVVQbeMGWSBrLsuRMsEc2XjsKYYDTKfoHCd4RGJtk3lXTUmJa z5p3i7T5srGaTBcngbzSlvZ3oFTOh6qrtSm5PIx8ttUaKp4E0EHBq3KMdURZIPu9aGNb 0PlCIoG9rWvdfeznMTpkYnXbIB22q3cqNx/zJXm+GoYuAOXS7faPOqnV/5/kCEmGK9Sh /wfw== 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=+UPRTkC19Vn7vLSddbivXVgWM2L0V/UYiFOje2tTh0I=; b=o+1GMO+xdrixg1Bitrc/3KWEtVommQlfUTSgXw0fsYCg9dGFEPY63y5XeG8YGcSbIj bsP5y8QPc/HXbAQ6HXN3f66zenoP0OGJvRdkVWpwHnnTGeeRl9My0tBjhfDwwTCiJcJg t7ddWeaQqod15FPIb/xlZs9XgPVE4WbhHewsN1mKTruFab0kgbSlJnnTPpvMhWzsfwS0 1dxxkDjd0r6XIQn6w4WqOqjlYjq3kXRYGMjPFEGDltu1TYwHSylBf1NKPsQ3dld/GjOZ 4i+Hv0I27RZhDTnXcAWsuXF+paVYNHcsm//dUDKmKUw0RXjrKu4tO1jI5F7VoBrLOtTR A0jA== X-Gm-Message-State: AA+aEWaAKgqIFTPTD7tK6HPwPn24QASjSis7VBuKbeyjJvle9WD8PgIU jsrF14TZ/Q348PoJUplxVZiD0g== X-Google-Smtp-Source: AFSGD/WHwVB9X+q6JNQKTx2dl2pRXupo+B02pXDjRe45BDgiQ91D/QPMHTVNcDONRi5Xv4ty5Hjovw== X-Received: by 2002:a62:6b85:: with SMTP id g127mr3386142pfc.42.1544206213965; Fri, 07 Dec 2018 10:10:13 -0800 (PST) Received: from xeon-e3 (204-195-22-127.wavecable.com. [204.195.22.127]) by smtp.gmail.com with ESMTPSA id c7sm9627308pfa.24.2018.12.07.10.10.13 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Fri, 07 Dec 2018 10:10:13 -0800 (PST) Date: Fri, 7 Dec 2018 10:10:11 -0800 From: Stephen Hemminger To: Erik Gabriel Carrillo Cc: rsanford@akamai.com, jerin.jacob@caviumnetworks.com, pbhagavatula@caviumnetworks.com, dev@dpdk.org Message-ID: <20181207101011.4e0275b2@xeon-e3> In-Reply-To: <1544205180-31546-2-git-send-email-erik.g.carrillo@intel.com> References: <1543534514-183766-1-git-send-email-erik.g.carrillo@intel.com> <1544205180-31546-1-git-send-email-erik.g.carrillo@intel.com> <1544205180-31546-2-git-send-email-erik.g.carrillo@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v2 1/2] timer: allow timer management in shared memory 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 18:10:15 -0000 On Fri, 7 Dec 2018 11:52:59 -0600 Erik Gabriel Carrillo wrote: > Currently, the timer library uses a per-process table of structures to > manage skiplists of timers presumably because timers contain arbitrary > function pointers whose value may not resolve properly in other > processes. > > However, if the same callback is used handle all timers, and that > callback is only invoked in one process, then it woud be safe to allow > the data structures to be allocated in shared memory, and to allow > secondary processes to modify the timer lists. This would let timers be > used in more multi-process scenarios. > > The library's global variables are wrapped with a struct, and an array > of these structures is created in shared memory. The original APIs > are updated to reference the zeroth entry in the array. This maintains > the original behavior for both primary and secondary processes since > the set intersection of their coremasks should be empty [1]. New APIs > are introduced to enable the allocation/deallocation of other entries > in the array. > > New variants of the APIs used to start and stop timers are introduced; > they allow a caller to specify which array entry should be used to > locate the timer list to insert into or delete from. > > Finally, a new variant of rte_timer_manage() is introduced, which > allows a caller to specify which array entry should be used to locate > the timer lists to process; it can also process multiple timer lists per > invocation. > > [1] https://doc.dpdk.org/guides/prog_guide/multi_proc_support.html#multi-process-limitations > > Signed-off-by: Erik Gabriel Carrillo Makes sense but it looks to me like an ABI breakage. Experimental isn't going to work for this. > +static uint32_t default_data_id; // id set to zero automatically C++ style comments are not allowed per DPDK coding style. Best to just drop the comment, it is stating the obvious. > -/* Init the timer library. */ > +static inline int > +timer_data_valid(uint32_t id) > +{ > + return !!(rte_timer_data_arr[id].internal_flags & FL_ALLOCATED); > +} Don't need inline on static functions. ... > +MAP_STATIC_SYMBOL(int rte_timer_manage(void), rte_timer_manage_v1902); > +BIND_DEFAULT_SYMBOL(rte_timer_manage, _v1902, 19.02); > + > +int __rte_experimental > +rte_timer_alt_manage(uint32_t timer_data_id, > + unsigned int *poll_lcores, > + int nb_poll_lcores, > + rte_timer_alt_manage_cb_t f) > +{ > + union rte_timer_status status; > + struct rte_timer *tim, *next_tim, **pprev; > + struct rte_timer *run_first_tims[RTE_MAX_LCORE]; > + unsigned int runlist_lcore_ids[RTE_MAX_LCORE]; > + unsigned int this_lcore = rte_lcore_id(); > + struct rte_timer *prev[MAX_SKIPLIST_DEPTH + 1]; > + uint64_t cur_time; > + int i, j, ret; > + int nb_runlists = 0; > + struct rte_timer_data *data; > + struct priv_timer *privp; > + uint32_t poll_lcore; > + > + TIMER_DATA_VALID_GET_OR_ERR_RET(timer_data_id, data, -EINVAL); > + > + /* timer manager only runs on EAL thread with valid lcore_id */ > + assert(this_lcore < RTE_MAX_LCORE); > + > + __TIMER_STAT_ADD(data->priv_timer, manage, 1); > + > + if (poll_lcores == NULL) { > + poll_lcores = (unsigned int []){rte_lcore_id()}; This isn't going to be safe. It assigns poll_lcores to an array allocated on the stack. > + > + for (i = 0, poll_lcore = poll_lcores[i]; i < nb_poll_lcores; > + poll_lcore = poll_lcores[++i]) { > + privp = &data->priv_timer[poll_lcore]; > + > + /* optimize for the case where per-cpu list is empty */ > + if (privp->pending_head.sl_next[0] == NULL) > + continue; > + cur_time = rte_get_timer_cycles(); > + > +#ifdef RTE_ARCH_64 > + /* on 64-bit the value cached in the pending_head.expired will > + * be updated atomically, so we can consult that for a quick > + * check here outside the lock > + */ > + if (likely(privp->pending_head.expire > cur_time)) > + continue; > +#endif This code needs to be optimized so that application can call this at a very high rate without performance impact.