Hello,
On Thu, Apr 16, 2020 at 10:57:42AM +0200, Martin Pieuchot wrote:
> On 14/04/20(Tue) 10:08, Martin Pieuchot wrote:
> > On 13/04/20(Mon) 03:20, Alexandr Nedvedicky wrote:
> > > On Sun, Apr 12, 2020 at 07:02:43PM +0200, Mark Kettenis wrote:
> > > > > From: "Theo de Raadt" <dera...@openbsd.org>
> > > > > Date: Sun, 12 Apr 2020 10:28:59 -0600
> > > > >
> > > > > > + if ((p->p_flag & P_SYSTEM) &&
> > > > > > + (strncmp(p->p_p->ps_comm, "softnet", 7) == 0))
> > > > >
> > > > > Wow that is ugly.
> > > >
> > > > A better approach might be to store a pointer to the softnet task's
> > > > struct proc in a global variable and check that. That is what we do
> > > > for the pagedaemon for example.
> >
> > Diff below implements that by introducing the in_taskq() function, any
> > comment on that approach?
> >
> > [...]
> > Diff below uses NET_RLOCK_IN_SOFTNET() with a corresponding KASSERT()
> > and converts all the places where tasks are enqueued on the "softnet"
> > taskq.
>
> Do we consider this approach good enough to explicitly document the 2,5
> years old state of locking of the network stack?
having this ASSERT already would helped us to discover the bug reported
by Richard much faster and easier.
>
> Is the in_taskq() approach overkilled or is it fine? Or are the names
> good enough?
I like it more than my 'magic' flag idea I shared earlier.
I'm fine with names. They are explicit enough.
>
> Could this change have helped avoid the previous mistake?
I would say yes.
>
> In other words, are you ok with it?
I'm OK with it. However I would like to know some more details
on the construct below:
> @@ -356,7 +367,17 @@ taskq_thread(void *xtq)
> {
> struct taskq *tq = xtq;
> struct task work;
> - int last;
> + int last, i;
> +
> + mtx_enter(&tq->tq_mtx);
> + for (i = 0; i < tq->tq_nthreads; i++) {
> + if (tq->tq_threads[i] == NULL) {
> + tq->tq_threads[i] = curproc;
> + break;
> + }
> + }
> + KASSERT(i < tq->tq_nthreads);
> + mtx_leave(&tq->tq_mtx);
>
> if (ISSET(tq->tq_flags, TASKQ_MPSAFE))
> KERNEL_UNLOCK();
> @@ -381,4 +402,17 @@ taskq_thread(void *xtq)
> wakeup_one(&tq->tq_running);
>
> kthread_exit(0);
> +}
sorry if my question sounds stupid. The snippet above indicates
the task queue threads stay around forever (once created), right?
I'm asking, because I see no place where we do 'tq_threads[i] = NULL'.
thanks and
regards
sashan
