Use proper TAILQ functions for slpque

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

Use proper TAILQ functions for slpque

Christian Ludwig-3
Do not roll a custom for-loop, use the appropriate TAILQ function.

Also use unsleep() instead of coding the same functionality here again.
There is only one place in the system that resets p_wchan now. And the
unsleep-part has the same pattern like in endtsleep().
---
 sys/kern/kern_synch.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/sys/kern/kern_synch.c b/sys/kern/kern_synch.c
index 684624428db..154686e924b 100644
--- a/sys/kern/kern_synch.c
+++ b/sys/kern/kern_synch.c
@@ -436,15 +436,13 @@ unsleep(struct proc *p)
 void
 wakeup_n(const volatile void *ident, int n)
 {
- struct slpque *qp;
  struct proc *p;
- struct proc *pnext;
  int s;
 
  SCHED_LOCK(s);
- qp = &slpque[LOOKUP(ident)];
- for (p = TAILQ_FIRST(qp); p != NULL && n != 0; p = pnext) {
- pnext = TAILQ_NEXT(p, p_runq);
+ TAILQ_FOREACH(p, &slpque[LOOKUP(ident)], p_runq) {
+ if (n == 0)
+ break;
 #ifdef DIAGNOSTIC
  /*
  * If the rwlock passed to rwsleep() is contended, the
@@ -460,10 +458,10 @@ wakeup_n(const volatile void *ident, int n)
 #endif
  if (p->p_wchan == ident) {
  --n;
- p->p_wchan = 0;
- TAILQ_REMOVE(qp, p, p_runq);
  if (p->p_stat == SSLEEP)
  setrunnable(p);
+ else
+ unsleep(p);
  }
  }
  SCHED_UNLOCK(s);
--
2.19.0

Reply | Threaded
Open this post in threaded view
|

Re: Use proper TAILQ functions for slpque

Claudio Jeker
On Tue, Oct 16, 2018 at 08:31:14AM +0200, Christian Ludwig wrote:

> Do not roll a custom for-loop, use the appropriate TAILQ function.
>
> Also use unsleep() instead of coding the same functionality here again.
> There is only one place in the system that resets p_wchan now. And the
> unsleep-part has the same pattern like in endtsleep().
> ---
>  sys/kern/kern_synch.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/sys/kern/kern_synch.c b/sys/kern/kern_synch.c
> index 684624428db..154686e924b 100644
> --- a/sys/kern/kern_synch.c
> +++ b/sys/kern/kern_synch.c
> @@ -436,15 +436,13 @@ unsleep(struct proc *p)
>  void
>  wakeup_n(const volatile void *ident, int n)
>  {
> - struct slpque *qp;
>   struct proc *p;
> - struct proc *pnext;
>   int s;
>  
>   SCHED_LOCK(s);
> - qp = &slpque[LOOKUP(ident)];
> - for (p = TAILQ_FIRST(qp); p != NULL && n != 0; p = pnext) {
> - pnext = TAILQ_NEXT(p, p_runq);
> + TAILQ_FOREACH(p, &slpque[LOOKUP(ident)], p_runq) {
> + if (n == 0)
> + break;

I have big doubts about this change. There is a TAILQ_REMOVE inside this
loop so using TAILQ_FOREACH() is not right.

>  #ifdef DIAGNOSTIC
>   /*
>   * If the rwlock passed to rwsleep() is contended, the
> @@ -460,10 +458,10 @@ wakeup_n(const volatile void *ident, int n)
>  #endif
>   if (p->p_wchan == ident) {
>   --n;
> - p->p_wchan = 0;
> - TAILQ_REMOVE(qp, p, p_runq);
>   if (p->p_stat == SSLEEP)
>   setrunnable(p);
> + else
> + unsleep(p);
>   }
>   }
>   SCHED_UNLOCK(s);
> --
> 2.19.0
>

--
:wq Claudio

Reply | Threaded
Open this post in threaded view
|

Re: Use proper TAILQ functions for slpque

Mark Kettenis
In reply to this post by Christian Ludwig-3
> From: Christian Ludwig <[hidden email]>
> Date: Tue, 16 Oct 2018 08:31:14 +0200
>
> Do not roll a custom for-loop, use the appropriate TAILQ function.
>
> Also use unsleep() instead of coding the same functionality here again.
> There is only one place in the system that resets p_wchan now. And the
> unsleep-part has the same pattern like in endtsleep().

not ok.

You're removing the TAILQ_REMOVE in the SSLEEP case.  And since
unsleep() does a TAILQ_REMOVE() you can't use TAILQ_FOREACH() here.

> ---
>  sys/kern/kern_synch.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/sys/kern/kern_synch.c b/sys/kern/kern_synch.c
> index 684624428db..154686e924b 100644
> --- a/sys/kern/kern_synch.c
> +++ b/sys/kern/kern_synch.c
> @@ -436,15 +436,13 @@ unsleep(struct proc *p)
>  void
>  wakeup_n(const volatile void *ident, int n)
>  {
> - struct slpque *qp;
>   struct proc *p;
> - struct proc *pnext;
>   int s;
>  
>   SCHED_LOCK(s);
> - qp = &slpque[LOOKUP(ident)];
> - for (p = TAILQ_FIRST(qp); p != NULL && n != 0; p = pnext) {
> - pnext = TAILQ_NEXT(p, p_runq);
> + TAILQ_FOREACH(p, &slpque[LOOKUP(ident)], p_runq) {
> + if (n == 0)
> + break;
>  #ifdef DIAGNOSTIC
>   /*
>   * If the rwlock passed to rwsleep() is contended, the
> @@ -460,10 +458,10 @@ wakeup_n(const volatile void *ident, int n)
>  #endif
>   if (p->p_wchan == ident) {
>   --n;
> - p->p_wchan = 0;
> - TAILQ_REMOVE(qp, p, p_runq);
>   if (p->p_stat == SSLEEP)
>   setrunnable(p);
> + else
> + unsleep(p);
>   }
>   }
>   SCHED_UNLOCK(s);
> --
> 2.19.0
>
>

Reply | Threaded
Open this post in threaded view
|

[v2] Use TAILQ functions for slpque

Christian Ludwig-3
In reply to this post by Christian Ludwig-3
Do not roll our own for-loop, use the proper TAILQ function.

Also use unsleep() instead of unrolling the same functionality here again.
Now there is only one place that resets p_wchan. And the unsleep-part has
the same pattern like in endtsleep().
---

v2:
 - TAILQ_FOREACH -> TAILQ_FOREACH_SAFE, since TAILQ_REMOVE might be
   called. Silly me.

 sys/kern/kern_synch.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/sys/kern/kern_synch.c b/sys/kern/kern_synch.c
index 684624428db..a1689765ff9 100644
--- a/sys/kern/kern_synch.c
+++ b/sys/kern/kern_synch.c
@@ -436,15 +436,14 @@ unsleep(struct proc *p)
 void
 wakeup_n(const volatile void *ident, int n)
 {
- struct slpque *qp;
  struct proc *p;
  struct proc *pnext;
  int s;
 
  SCHED_LOCK(s);
- qp = &slpque[LOOKUP(ident)];
- for (p = TAILQ_FIRST(qp); p != NULL && n != 0; p = pnext) {
- pnext = TAILQ_NEXT(p, p_runq);
+ TAILQ_FOREACH_SAFE(p, &slpque[LOOKUP(ident)], p_runq, pnext) {
+ if (n == 0)
+ break;
 #ifdef DIAGNOSTIC
  /*
  * If the rwlock passed to rwsleep() is contended, the
@@ -460,10 +459,10 @@ wakeup_n(const volatile void *ident, int n)
 #endif
  if (p->p_wchan == ident) {
  --n;
- p->p_wchan = 0;
- TAILQ_REMOVE(qp, p, p_runq);
  if (p->p_stat == SSLEEP)
  setrunnable(p);
+ else
+ unsleep(p);
  }
  }
  SCHED_UNLOCK(s);
--
2.19.1