GNU bug report logs - #79334
[PATCH] Don't release thread select lock unnecessarily

Previous Next

Full log

View this message in rfc822 format

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 79334 <at> debbugs.gnu.org, eggert <at> cs.ucla.edu, dmitry <at> gutov.dev
Subject: bug#79334: [PATCH] Don't release thread select lock unnecessarily
Date: Fri, 29 Aug 2025 10:49:34 -0400

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Spencer Baugh <sbaugh <at> janestreet.com>
>> Cc: 79334 <at> debbugs.gnu.org,  eggert <at> cs.ucla.edu,  dmitry <at> gutov.dev
>> Date: Fri, 29 Aug 2025 09:45:07 -0400
>> 
>> Eli Zaretskii <eliz <at> gnu.org> writes:
>> 
>> >> That being said, I'm confident that it's incorrect and unnecessary to be
>> >> using thread_select instead of pselect here, so we should stop doing
>> >> that in addition to any other fix.
>> >
>> > If we fix status_notify, what other reasons remain for not calling
>> > thread_select there?
>> 
>> Primarily: Calling thread_select here complicates reasoning about the
>> code.  We should not release the lock when not necessary, because it
>> makes it harder to think about possible thread interleavings.  And it's
>> not necessary here because this pselect call returns immediately.
>
> How do we define "necessary" in this context?

It's necessary to release the lock around long pselect calls because
otherwise other threads will not run while Emacs is waiting for input,
which means they probably won't run at all.

>> (The only reason the pselect call wouldn't return immediately is if we
>> released the lock and then switched to another thread)
>
> You seem to assume that a call to thread_select is only necessary when
> some potentially prolonged operation or wait is expected?  But I'm not
> sure this is correct, because that could prevent other threads from
> running for too long, and force programmers to inject sleep-for
> etc. in their programs.  By allowing thread switches as frequently as
> possible lets other threads more opportunities to run (to some extent
> simulating preemptive scheduling), which I think is a Good Thing
> overall, no?

Three points:

- Most importantly: We can only allow thread switches at known safe
  points.  It's not clear that this is a safe point.  (Actually, we know
  it currently isn't, because of the status_notify issue.  But I suspect
  there may be other issues which are harder to analyze)

- Adding more thread switches is not necessarily a good thing.  It can
  degrade whole-system performance, because context switching is slow.

- wait_reading_process_output already does a thread switch around the
  pselect, so it's not important for one to happen here.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.