cvs commit: src/sys/kern kern_sig.c

[David Schultz]

On Thu, Mar 03, 2005, David Xu wrote:
> David Schultz wrote:
> 
> >On Thu, Mar 03, 2005, David Xu wrote:
> > 
> >
> >>John Baldwin wrote:
> >>
> >>   
> >>
> >>>On Wednesday 02 March 2005 08:43 am, David Xu wrote:
> >>>
> >>>
> >>>     
> >>>
> >>>>davidxu     2005-03-02 13:43:51 UTC
> >>>>
> >>>>FreeBSD src repository
> >>>>
> >>>>Modified files:
> >>>> sys/kern             kern_sig.c
> >>>>Log:
> >>>>In kern_sigtimedwait, remove waitset bits for td_sigmask before
> >>>>sleeping, so in do_tdsignal, we no longer need to test td_waitset.
> >>>>now td_waitset is only used to give a thread higher priority when
> >>>>delivering signal to multithreads process.
> >>>>This also fixes a bug:
> >>>>when a thread in sigwait states was suspended and later resumed
> >>>>by SIGCONT, it can no longer receive signals belong to waitset.
> >>>> 
> >>>>
> >>>>       
> >>>>
> >>>Is this related at all to Peter Holm's panic where sigwait() + swapping 
> >>>invokes a panic?
> >>>
> >>>
> >>>
> >>>     
> >>>
> >>No. Peter Holm's found is a swapping problem. vm swaps out sleeping
> >>thread's stack under memory stressing case. but I think that's not safe,
> >>that means, following code can not be used in kernel:
> >>
> >>int *p;
> >>
> >>func()
> >>{
> >>  int n;
> >>
> >>  n = 0;
> >>  p = &n;
> >>  msleep(p);
> >>  /* check variable n ...
> >>}
> >>
> >>func2()
> >>{
> >> *p = 2;
> >> wakeup(p);
> >>}
> >>
> >>unless million lines of kernel code are reviewed, I don't think the
> >>vm code is safe. The following patch should avoid the problem:
> >>   
> >>
> >[...]
> >
> >KSE already mostly broke swapping, so I'm not sure we need to
> >break it some more.  I think a better fix would be to mark threads
> >as unswappable in msleep() and cv_wait().  There would probably
> >need to be a separate msleep_swapok() for places where swapping
> >the process out is okay.  (IIRC, Solaris has something like this,
> >but they use it because their cv_wait() works with locks held, and
> >so the swapok variant is for situations where no locks are held.)
> >
> > 
> >
> This only partly resolves the problem,  if function A call B, B call C,  
> C is unknown to A,
> and C does a msleep(),  problem still lhappens.
> However, if there needs a flag,  I would like  PNOSWAP for msleep just 
> like PCATCH
> does.

You have to worry about that anyway, though.  A and B need to know
that they're not allowed to hold locks across the calls if C calls
msleep(), for instance.  Anyway, your proposal if having a flag
for msleep() is basically the same as my proposal of having a
separate function.  (The only difference is that adding a separate
function doesn't break the ABI.)  So it sounds like we're more or
less in agreement here.

> >The alternative, of course, is to just fix the code that assumes
> >that swapping doesn't exist.
> >
> First find all code written in such way, but it is not that easy.

True.  If we changed msleep() to disable swapping by default, then
we wouldn't have to worry about correctness problems related to
missing some.