Date: Thu, 11 Jul 2002 09:59:13 -0500 From: Jonathan Lemon <jlemon@flugsvamp.com> To: Archie Cobbs <archie@dellroad.org> Cc: Jonathan Lemon <jlemon@flugsvamp.com>, John Baldwin <jhb@FreeBSD.ORG>, davidx@viasoft.com.cn, freebsd-arch@FreeBSD.ORG, julian@elischer.org Subject: Re: Timeout and SMP race Message-ID: <20020711095913.H65393@prism.flugsvamp.com> In-Reply-To: <200207110025.g6B0PHA30341@arch20m.dellroad.org> References: <20020710171552.F65393@prism.flugsvamp.com> <200207110025.g6B0PHA30341@arch20m.dellroad.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, Jul 10, 2002 at 05:25:17PM -0700, Archie Cobbs wrote:
> Jonathan Lemon writes:
> > > extern int timer_start(timer_handle_t *handlep, mutex_t *mutexp,
> > > timer_func_t tfunc, void *arg, u_int delay,
> > > int flags);
> > > extern void timer_cancel(timer_handle_t *handlep);
> > > extern int timer_remaining(timer_handle_t handle, u_int *delayp);
> >
> > It seems to me that we can achieve the same functionality without
> > changing the callout API too much. You mention that the mutex (if supplied)
> > will be acquired before calling tfunc. This means that it has to be
> > stored somewhere, presumably in the callout structure itself.
> >
> > The callout() consumers typically allocate their own storage, so perhaps
> > we should add an (optional) pointer to a mutex to the callout structure,
> > where the mutex is obtained/released before the callout function is made.
>
> Yep, that would work too.. essentially it's the same thing.
>
> If you're doing that, why not just store the mutex itself in the
> callout structure, rather than a pointer to it? I guess if you did
> that then you would then need some kind of flag that says whether
> to use it or not. Or.. maybe there would be some way for the timer
> code to tell if the mutex has been initialized or not, and use this
> to decide whether to use it or not?
Well, the other day, I was thinking we have this current API:
callout_init(struct callout *c, int mpsafe)
where 'mpsafe' indicates whether the callout code should grab the Giant
lock or not. It wouldn't seem too much of a stretch to change this to:
callout_init(struct callout *c, struct mtx *m)
(perhaps redundant, since *m will be stored in the callout structure).
Now, this removes the special cases dealing with 'Giant' lock in the
softclock, since those callers who need the Giant lock would be able
to specify it themselves. For TCP timeouts, all of the timeouts would
point to the lock on the structure:
/* cut down pseudocode */
struct inp_tp {
struct mtx inp_mtx;
struct callout inp_tp_rexmt;
struct callout inp_tp_persist;
struct callout inp_tp_keep;
struct callout inp_tp_delack;
}
callout_init(&inp->inp_tp_rexmt, &inp->inp_mtx);
callout_init(&inp->inp_tp_persist, &inp->inp_mtx);
callout_init(&inp->inp_tp_keep, &inp->inp_mtx);
callout_init(&inp->inp_tp_delack, &inp->inp_mtx);
So TCP code, which grabs the inp_mtx for the duration, would be
protected against the timer, and no additional locking is needed.
The timer code would grab the appropriate lock before calling the
timeout. I wouldn't want an individual mutex in each timer, as by
storing a pointer, I can obtain/protect the appropriate resource,
which extends further than just the callout structure.
--
Jonathan
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-arch" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20020711095913.H65393>