From: "Carrillo, Erik G" <erik.g.carrillo@intel.com>
To: Stephen Hemminger <stephen@networkplumber.org>
Cc: "rsanford@akamai.com" <rsanford@akamai.com>,
"jerin.jacob@caviumnetworks.com" <jerin.jacob@caviumnetworks.com>,
"pbhagavatula@caviumnetworks.com"
<pbhagavatula@caviumnetworks.com>, "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v2 1/2] timer: allow timer management in shared memory
Date: Fri, 7 Dec 2018 19:21:55 +0000 [thread overview]
Message-ID: <BE54F058557D9A4FAC1D84E2FC6D87572334F3BB@fmsmsx115.amr.corp.intel.com> (raw)
In-Reply-To: <20181207101011.4e0275b2@xeon-e3>
Hi Stephen,
Thanks for the review. Some responses in-line:
> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Friday, December 7, 2018 12:10 PM
> To: Carrillo, Erik G <erik.g.carrillo@intel.com>
> Cc: rsanford@akamai.com; jerin.jacob@caviumnetworks.com;
> pbhagavatula@caviumnetworks.com; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 1/2] timer: allow timer management in
> shared memory
>
> On Fri, 7 Dec 2018 11:52:59 -0600
> Erik Gabriel Carrillo <erik.g.carrillo@intel.com> 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-
> p
> > rocess-limitations
> >
> > Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
>
> Makes sense but it looks to me like an ABI breakage. Experimental isn't going
> to work for this.
For APIs that existed prior to this patch, I've duplicated them in a "19.02" node in
the map file; I only marked new APIs as experimental. I versioned each API in
order to maintain the prior interface as well. I tested ABI compatibility
with devtools/validate-abi.sh; it reported no errors detected. So I believe this
won't break the ABI, but if I need to change something I certainly will.
>
> > +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.
>
Sure - will do.
> > -/* 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.
>
poll_lcores is allowed to be NULL when rte_timer_alt_manage() is called for
convenience; if it is NULL, then we create an array on the stack
containing one item and point poll_lcores at it. poll_lcores only needs to be
valid for the invocation of the function, so pointing to an array on the stack
seems fine. Did I miss the point?
> > +
> > + 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.
next prev parent reply other threads:[~2018-12-07 19:21 UTC|newest]
Thread overview: 77+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-11-29 23:35 [dpdk-dev] [PATCH 0/3] new software event timer adapter Erik Gabriel Carrillo
2018-11-29 23:35 ` [dpdk-dev] [PATCH 1/3] timer: allow timer management in shared memory Erik Gabriel Carrillo
2018-11-29 23:35 ` [dpdk-dev] [PATCH 2/3] timer: add function to stop all timers in a list Erik Gabriel Carrillo
2018-11-29 23:35 ` [dpdk-dev] [PATCH 3/3] eventdev: add new software event timer adapter Erik Gabriel Carrillo
2018-11-30 7:26 ` [dpdk-dev] [PATCH 0/3] " Pavan Nikhilesh
2018-11-30 19:07 ` Carrillo, Erik G
2018-12-07 17:52 ` [dpdk-dev] [PATCH v2 0/2] Timer library changes Erik Gabriel Carrillo
2018-12-07 17:52 ` [dpdk-dev] [PATCH v2 1/2] timer: allow timer management in shared memory Erik Gabriel Carrillo
2018-12-07 18:10 ` Stephen Hemminger
2018-12-07 19:21 ` Carrillo, Erik G [this message]
2018-12-07 17:53 ` [dpdk-dev] [PATCH v2 2/2] timer: add function to stop all timers in a list Erik Gabriel Carrillo
2018-12-13 22:26 ` [dpdk-dev] [PATCH v3 0/2] Timer library changes Erik Gabriel Carrillo
2018-12-13 22:26 ` [dpdk-dev] [PATCH v3 1/2] timer: allow timer management in shared memory Erik Gabriel Carrillo
2018-12-13 22:26 ` [dpdk-dev] [PATCH v3 2/2] timer: add function to stop all timers in a list Erik Gabriel Carrillo
2018-12-19 3:35 ` [dpdk-dev] [PATCH v3 0/2] Timer library changes Thomas Monjalon
2018-12-19 7:33 ` Mattias Rönnblom
2019-03-05 22:41 ` Carrillo, Erik G
2019-03-05 22:58 ` [dpdk-dev] [dpdk-techboard] " Thomas Monjalon
2019-03-06 18:54 ` Carrillo, Erik G
2019-03-06 20:17 ` Thomas Monjalon
2019-03-06 2:39 ` [dpdk-dev] " Varghese, Vipin
2019-03-06 15:15 ` Carrillo, Erik G
2019-03-07 2:33 ` Varghese, Vipin
2019-03-06 17:20 ` [dpdk-dev] [PATCH v4 " Erik Gabriel Carrillo
2019-03-06 17:20 ` [dpdk-dev] [PATCH v4 1/2] timer: allow timer management in shared memory Erik Gabriel Carrillo
2019-03-20 13:52 ` Sanford, Robert
2019-03-20 13:52 ` Sanford, Robert
2019-03-21 1:01 ` Carrillo, Erik G
2019-03-21 1:01 ` Carrillo, Erik G
2019-03-27 14:03 ` Thomas Monjalon
2019-03-27 14:03 ` Thomas Monjalon
2019-03-28 12:42 ` Carrillo, Erik G
2019-03-28 12:42 ` Carrillo, Erik G
2019-04-15 21:49 ` Carrillo, Erik G
2019-04-15 21:49 ` Carrillo, Erik G
2019-03-06 17:20 ` [dpdk-dev] [PATCH v4 2/2] timer: add function to stop all timers in a list Erik Gabriel Carrillo
2019-04-15 21:41 ` [dpdk-dev] [PATCH v5 0/2] Timer library changes Erik Gabriel Carrillo
2019-04-15 21:41 ` Erik Gabriel Carrillo
2019-04-15 21:41 ` [dpdk-dev] [PATCH v5 1/2] timer: allow timer management in shared memory Erik Gabriel Carrillo
2019-04-15 21:41 ` Erik Gabriel Carrillo
2019-04-17 17:09 ` Thomas Monjalon
2019-04-17 17:09 ` Thomas Monjalon
2019-04-15 21:41 ` [dpdk-dev] [PATCH v5 2/2] timer: add function to stop all timers in a list Erik Gabriel Carrillo
2019-04-15 21:41 ` Erik Gabriel Carrillo
2019-04-17 19:54 ` [dpdk-dev] [PATCH v5 0/2] Timer library changes Thomas Monjalon
2019-04-17 19:54 ` Thomas Monjalon
2018-12-07 20:34 ` [dpdk-dev] [PATCH v2 0/1] New software event timer adapter Erik Gabriel Carrillo
2018-12-07 20:34 ` [dpdk-dev] [PATCH v2 1/1] eventdev: add new " Erik Gabriel Carrillo
2018-12-09 19:17 ` Mattias Rönnblom
2018-12-10 17:17 ` Carrillo, Erik G
2018-12-14 15:45 ` [dpdk-dev] [PATCH v3 0/1] New " Erik Gabriel Carrillo
2018-12-14 15:45 ` [dpdk-dev] [PATCH v3 1/1] eventdev: add new " Erik Gabriel Carrillo
2018-12-14 21:15 ` Mattias Rönnblom
2018-12-14 23:04 ` Carrillo, Erik G
2018-12-14 23:15 ` [dpdk-dev] [PATCH v4 0/1] New " Erik Gabriel Carrillo
2018-12-14 23:15 ` [dpdk-dev] [PATCH v4 1/1] eventdev: add new " Erik Gabriel Carrillo
2018-12-18 20:11 ` [dpdk-dev] [EXT] [PATCH v4 0/1] New " Jerin Jacob Kollanukkaran
2018-12-18 20:14 ` Carrillo, Erik G
2019-04-22 14:57 ` [dpdk-dev] [PATCH v5 " Erik Gabriel Carrillo
2019-04-22 14:57 ` Erik Gabriel Carrillo
2019-04-22 14:57 ` [dpdk-dev] [PATCH v5 1/1] eventdev: add new " Erik Gabriel Carrillo
2019-04-22 14:57 ` Erik Gabriel Carrillo
2019-04-26 15:14 ` [dpdk-dev] [PATCH v6 0/1] New " Erik Gabriel Carrillo
2019-04-26 15:14 ` Erik Gabriel Carrillo
2019-04-26 15:14 ` [dpdk-dev] [PATCH v6 1/1] eventdev: add new " Erik Gabriel Carrillo
2019-04-26 15:14 ` Erik Gabriel Carrillo
2019-04-26 18:51 ` Honnappa Nagarahalli
2019-04-26 18:51 ` Honnappa Nagarahalli
2019-04-26 18:58 ` Carrillo, Erik G
2019-04-26 18:58 ` Carrillo, Erik G
2019-06-05 13:34 ` Jerin Jacob Kollanukkaran
2019-06-19 15:14 ` [dpdk-dev] [PATCH v7 0/1] New " Erik Gabriel Carrillo
2019-06-19 15:14 ` [dpdk-dev] [PATCH v7 1/1] eventdev: add new " Erik Gabriel Carrillo
2019-06-19 16:25 ` [dpdk-dev] [PATCH v8 0/1] New " Erik Gabriel Carrillo
2019-06-19 16:25 ` [dpdk-dev] [PATCH v8 1/1] eventdev: add new " Erik Gabriel Carrillo
2019-06-24 6:12 ` Jerin Jacob Kollanukkaran
2019-06-25 6:06 ` Jerin Jacob Kollanukkaran
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=BE54F058557D9A4FAC1D84E2FC6D87572334F3BB@fmsmsx115.amr.corp.intel.com \
--to=erik.g.carrillo@intel.com \
--cc=dev@dpdk.org \
--cc=jerin.jacob@caviumnetworks.com \
--cc=pbhagavatula@caviumnetworks.com \
--cc=rsanford@akamai.com \
--cc=stephen@networkplumber.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).