Mateusz Guzik <mjguzik@xxxxxxxxx> writes: > On Fri, Jan 31, 2025 at 9:56 PM Eric W. Biederman <ebiederm@xxxxxxxxxxxx> wrote: >> Moving proc_flush_pid inside of tasklist_lock is a bad idea. > > The patch does not make such a change though. > > The call is still performed without the lock, but it also dodges the > additional refcount dance (and notably eliminates an atomic from an > area protected by tasklist_lock). My mistake I saw you had moved it up, but I had missed just how far. It is still a bad idea to move it early, as that has caused problems with lingering proc entries for reaped task clogging up the dcache. >> It is wrong that attach_pid/detach_pid can be performed without the >> tasklist_lock. There are reasonable guarantees provided by the posix >> standard that the set of processes sent a signal is the set of >> processes at a point in time. The tasklist_lock is how we provide >> those guarantees currently. >> > > I don't see anything calling these without the lock and neither my > patch nor a follow up about pids suggest anything of the sort. No. You simply said fork called attach_pid without the lock and your description implied it was safe to call attach_pid/detach_pid without the lock. >> There are two more layers to pids. The pid number allocation of >> alloc_pid/free_pid, and the struct pid layer maintained by get_pid, >> put_pid. Those two layers don't need the tasklist_lock. >> >> >> It is safe to move free_pid out of tasklist_lock. I am not certain >> how sane it is. >> > > Where is the sanity problem here? AFAICS this just delays some wakeups > in the worst case. At the end of the day it might be a sensible way to go. I just haven't found the arguments to convince my gut of that yet. It is important for me at least to convince my gut because it usually notices problems before the rest of me does. Eric
