From: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
To: Neil Horman <nhorman@tuxdriver.com>,
"Jastrzebski, MichalX K" <michalx.k.jastrzebski@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v2] Change alarm cancel function to thread-safe:
Date: Thu, 25 Sep 2014 16:03:48 +0000 [thread overview]
Message-ID: <2601191342CEEE43887BDE71AB977258213769DE@IRSMSX105.ger.corp.intel.com> (raw)
In-Reply-To: <20140925150807.GD32725@hmsreliant.think-freely.org>
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Neil Horman
> Sent: Thursday, September 25, 2014 4:08 PM
> To: Jastrzebski, MichalX K
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2] Change alarm cancel function to thread-safe:
>
> On Thu, Sep 25, 2014 at 01:56:08PM +0100, Michal Jastrzebski wrote:
> > Change alarm cancel function to thread-safe.
> > It eliminates a race between threads using rte_alarm_cancel and
> > rte_alarm_set.
> >
> > Signed-off-by: Pawel Wodkowski <pawelx.wodkowski@intel.com>
> > Reviewed-by: Michal Jastrzebski <michalx.k.jastrzebski@intel.com>
> >
> > ---
> > lib/librte_eal/common/include/rte_alarm.h | 3 +-
> > lib/librte_eal/linuxapp/eal/eal_alarm.c | 68 ++++++++++++++++++-----------
> > 2 files changed, 45 insertions(+), 26 deletions(-)
> >
>
> > diff --git a/lib/librte_eal/common/include/rte_alarm.h b/lib/librte_eal/common/include/rte_alarm.h
> > index d451522..e7cbaef 100644
> > --- a/lib/librte_eal/common/include/rte_alarm.h
> > +++ b/lib/librte_eal/common/include/rte_alarm.h
> > @@ -76,7 +76,8 @@ typedef void (*rte_eal_alarm_callback)(void *arg);
> > int rte_eal_alarm_set(uint64_t us, rte_eal_alarm_callback cb, void *cb_arg);
> >
> > /**
> > - * Function to cancel an alarm callback which has been registered before.
> > + * Function to cancel an alarm callback which has been registered before. If
> > + * used outside alarm callback it wait for all callbacks to finish its execution.
> > *
> > * @param cb_fn
> > * alarm callback
> > diff --git a/lib/librte_eal/linuxapp/eal/eal_alarm.c b/lib/librte_eal/linuxapp/eal/eal_alarm.c
> > index 480f0cb..ea8dfb4 100644
> > --- a/lib/librte_eal/linuxapp/eal/eal_alarm.c
> > +++ b/lib/librte_eal/linuxapp/eal/eal_alarm.c
> > @@ -69,7 +69,8 @@ struct alarm_entry {
> > struct timeval time;
> > rte_eal_alarm_callback cb_fn;
> > void *cb_arg;
> > - volatile int executing;
> > + volatile uint8_t executing;
> > + volatile pthread_t executing_id;
> > };
> >
> > static LIST_HEAD(alarm_list, alarm_entry) alarm_list = LIST_HEAD_INITIALIZER();
> > @@ -108,11 +109,13 @@ eal_alarm_callback(struct rte_intr_handle *hdl __rte_unused,
> > (ap->time.tv_sec < now.tv_sec || (ap->time.tv_sec == now.tv_sec &&
> > ap->time.tv_usec <= now.tv_usec))){
> > ap->executing = 1;
> > + ap->executing_id = pthread_self();
> How exactly does this work? From my read all alarm callbacks are handled by the
> thread created in rte_eal_intr_init (which runs forever in
> eal_intr_thread_main()).
In current implementation - yes.
So every assignment to the above executing_id value
> will be from that thread. As such, anytime rte_eal_alarm_cancel is called from
> within a callback we are guaranteed that:
> a) the ap->executing flag is set to 1
> b) the ap->executing_id value should equal whatever is returned from
> pthread_self()
Yes
>
> That will cause the executing counter local to the cancel function to get
> incremented, meaning we will deadlock withing that do { ... } while (executing
> != 0) loop, no?
No, as for the case when cancel is called from callback:
pthread_equal(ap->executing_id, pthread_self())
would return non-zero value (which means threads ids are equal), so executing will not be incremented.
>
> > rte_spinlock_unlock(&alarm_list_lk);
> >
> > ap->cb_fn(ap->cb_arg);
> >
> > rte_spinlock_lock(&alarm_list_lk);
> > +
> > LIST_REMOVE(ap, next);
> > rte_free(ap);
> > }
> > @@ -145,7 +148,7 @@ rte_eal_alarm_set(uint64_t us, rte_eal_alarm_callback cb_fn, void *cb_arg)
> > if (us < 1 || us > (UINT64_MAX - US_PER_S) || cb_fn == NULL)
> > return -EINVAL;
> >
> > - new_alarm = rte_malloc(NULL, sizeof(*new_alarm), 0);
> > + new_alarm = rte_zmalloc(NULL, sizeof(*new_alarm), 0);
> > if (new_alarm == NULL)
> > return -ENOMEM;
> >
> > @@ -156,7 +159,6 @@ rte_eal_alarm_set(uint64_t us, rte_eal_alarm_callback cb_fn, void *cb_arg)
> > new_alarm->cb_arg = cb_arg;
> > new_alarm->time.tv_usec = (now.tv_usec + us) % US_PER_S;
> > new_alarm->time.tv_sec = now.tv_sec + ((now.tv_usec + us) / US_PER_S);
> > - new_alarm->executing = 0;
> >
> This removes the only place where ->executing is cleared again. If there is
> only one change to this bits state (which is the case after this patch), it
> seems that you can just use the executing bit as the test in the alarm_cancel
> function, and remove all the pthread_self mess.
I believe we do need executing_id here.
It allows us to distinguish are we executing cancel from a callback or not.
> We still need to address the
> deadlock question, but it seems using this single flag is easier than using
> pthread_self.
>
> Neil
next prev parent reply other threads:[~2014-09-25 16:00 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-09-25 12:56 Michal Jastrzebski
2014-09-25 13:11 ` Ananyev, Konstantin
2014-09-25 15:08 ` Neil Horman
2014-09-25 16:03 ` Ananyev, Konstantin [this message]
2014-09-25 17:23 ` Neil Horman
2014-09-25 23:24 ` Ananyev, Konstantin
2014-09-26 11:46 ` Neil Horman
2014-09-26 12:37 ` Wodkowski, PawelX
2014-09-26 13:40 ` Neil Horman
2014-09-26 14:01 ` Wodkowski, PawelX
2014-09-26 15:01 ` Neil Horman
2014-09-26 15:41 ` Ananyev, Konstantin
2014-09-26 16:21 ` Neil Horman
2014-09-26 18:07 ` Ananyev, Konstantin
2014-09-26 19:39 ` Neil Horman
2014-09-28 16:12 ` Ananyev, Konstantin
2014-09-28 20:47 ` Neil Horman
2014-09-29 6:40 ` Wodkowski, PawelX
2014-09-29 9:50 ` Ananyev, Konstantin
2014-09-29 10:11 ` Wodkowski, PawelX
2014-09-29 10:33 ` Bruce Richardson
2014-09-30 11:13 ` Wodkowski, PawelX
2014-09-30 12:05 ` Wodkowski, PawelX
2014-09-30 12:30 ` Ananyev, Konstantin
2014-09-30 12:54 ` Neil Horman
2014-09-29 11:35 ` Neil Horman
2014-09-26 14:13 ` Ananyev, Konstantin
2014-09-29 10:37 ` Bruce Richardson
2014-09-26 6:33 ` Wodkowski, PawelX
2014-09-26 9:49 ` Wodkowski, PawelX
2014-09-26 13:43 ` Neil Horman
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=2601191342CEEE43887BDE71AB977258213769DE@IRSMSX105.ger.corp.intel.com \
--to=konstantin.ananyev@intel.com \
--cc=dev@dpdk.org \
--cc=michalx.k.jastrzebski@intel.com \
--cc=nhorman@tuxdriver.com \
/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).