sb_flags vs sb_flagsintr

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

sb_flags vs sb_flagsintr

Martin Pieuchot
Diff below change the usage of `sb_flags' and `sb_flagsintr'.  The
former will be protected by the socket lock while the latter will
be using atomic operations.

- SB_SPLICE and SB_WAIT are always manipulated with solock() held, so
  move them to sb_flags.
- SB_SEL is also in the same case, although we might want to move it
  to sb_flagsintr later.
- SB_KNOTE remains the only bit set on sb_flagsintr.  Using atomic
  operation in this case should prevent us from sleeping in kevent(2)
  filters.

The diff below also includes some KERNEL_LOCK()/UNLOCK() dances around
selwakeup() and csignal().  These are currently no-op but mark which
functions need to be fixed to unlock the socket layer.

fifo_poll() was missing a solock/unlock dance, it is also included
in the diff below.

ok?

Index: kern/uipc_socket.c
===================================================================
RCS file: /cvs/src/sys/kern/uipc_socket.c,v
retrieving revision 1.209
diff -u -p -r1.209 uipc_socket.c
--- kern/uipc_socket.c 23 Nov 2017 13:45:46 -0000 1.209
+++ kern/uipc_socket.c 4 Dec 2017 14:26:24 -0000
@@ -1054,9 +1054,9 @@ sorflush(struct socket *so)
  aso.so_rcv = *sb;
  memset(sb, 0, sizeof (*sb));
  /* XXX - the memset stomps all over so_rcv */
- if (aso.so_rcv.sb_flags & SB_KNOTE) {
+ if (aso.so_rcv.sb_flagsintr & SB_KNOTE) {
  sb->sb_sel.si_note = aso.so_rcv.sb_sel.si_note;
- sb->sb_flags = SB_KNOTE;
+ sb->sb_flagsintr = SB_KNOTE;
  }
  if (pr->pr_flags & PR_RIGHTS && pr->pr_domain->dom_dispose)
  (*pr->pr_domain->dom_dispose)(aso.so_rcv.sb_mb);
@@ -1178,8 +1178,8 @@ sosplice(struct socket *so, int fd, off_
  * we sleep, the socket buffers are not marked as spliced yet.
  */
  if (somove(so, M_WAIT)) {
- so->so_rcv.sb_flagsintr |= SB_SPLICE;
- sosp->so_snd.sb_flagsintr |= SB_SPLICE;
+ so->so_rcv.sb_flags |= SB_SPLICE;
+ sosp->so_snd.sb_flags |= SB_SPLICE;
  }
 
  release:
@@ -1196,8 +1196,8 @@ sounsplice(struct socket *so, struct soc
 
  task_del(sosplice_taskq, &so->so_splicetask);
  timeout_del(&so->so_idleto);
- sosp->so_snd.sb_flagsintr &= ~SB_SPLICE;
- so->so_rcv.sb_flagsintr &= ~SB_SPLICE;
+ sosp->so_snd.sb_flags &= ~SB_SPLICE;
+ so->so_rcv.sb_flags &= ~SB_SPLICE;
  so->so_sp->ssp_socket = sosp->so_sp->ssp_soback = NULL;
  if (wakeup && soreadable(so))
  sorwakeup(so);
@@ -1210,7 +1210,7 @@ soidle(void *arg)
  int s;
 
  s = solock(so);
- if (so->so_rcv.sb_flagsintr & SB_SPLICE) {
+ if (so->so_rcv.sb_flags & SB_SPLICE) {
  so->so_error = ETIMEDOUT;
  sounsplice(so, so->so_sp->ssp_socket, 1);
  }
@@ -1224,7 +1224,7 @@ sotask(void *arg)
  int s;
 
  s = solock(so);
- if (so->so_rcv.sb_flagsintr & SB_SPLICE) {
+ if (so->so_rcv.sb_flags & SB_SPLICE) {
  /*
  * We may not sleep here as sofree() and unsplice() may be
  * called from softnet interrupt context.  This would remove
@@ -1527,7 +1527,7 @@ sorwakeup(struct socket *so)
  soassertlocked(so);
 
 #ifdef SOCKET_SPLICE
- if (so->so_rcv.sb_flagsintr & SB_SPLICE) {
+ if (so->so_rcv.sb_flags & SB_SPLICE) {
  /*
  * TCP has a sendbuffer that can handle multiple packets
  * at once.  So queue the stream a bit to accumulate data.
@@ -1555,7 +1555,7 @@ sowwakeup(struct socket *so)
  soassertlocked(so);
 
 #ifdef SOCKET_SPLICE
- if (so->so_snd.sb_flagsintr & SB_SPLICE)
+ if (so->so_snd.sb_flags & SB_SPLICE)
  task_add(sosplice_taskq, &so->so_sp->ssp_soback->so_splicetask);
 #endif
  sowakeup(so, &so->so_snd);
@@ -1871,9 +1871,10 @@ sogetopt(struct socket *so, int level, i
 void
 sohasoutofband(struct socket *so)
 {
- KERNEL_ASSERT_LOCKED();
+ KERNEL_LOCK();
  csignal(so->so_pgid, SIGURG, so->so_siguid, so->so_sigeuid);
  selwakeup(&so->so_rcv.sb_sel);
+ KERNEL_UNLOCK();
 }
 
 int
@@ -1901,7 +1902,7 @@ soo_kqfilter(struct file *fp, struct kno
  }
 
  SLIST_INSERT_HEAD(&sb->sb_sel.si_note, kn, kn_selnext);
- sb->sb_flags |= SB_KNOTE;
+ sb->sb_flagsintr |= SB_KNOTE;
 
  return (0);
 }
@@ -1915,7 +1916,7 @@ filt_sordetach(struct knote *kn)
 
  SLIST_REMOVE(&so->so_rcv.sb_sel.si_note, kn, knote, kn_selnext);
  if (SLIST_EMPTY(&so->so_rcv.sb_sel.si_note))
- so->so_rcv.sb_flags &= ~SB_KNOTE;
+ so->so_rcv.sb_flagsintr &= ~SB_KNOTE;
 }
 
 int
@@ -1958,7 +1959,7 @@ filt_sowdetach(struct knote *kn)
 
  SLIST_REMOVE(&so->so_snd.sb_sel.si_note, kn, knote, kn_selnext);
  if (SLIST_EMPTY(&so->so_snd.sb_sel.si_note))
- so->so_snd.sb_flags &= ~SB_KNOTE;
+ so->so_snd.sb_flagsintr &= ~SB_KNOTE;
 }
 
 int
Index: kern/sys_socket.c
===================================================================
RCS file: /cvs/src/sys/kern/sys_socket.c,v
retrieving revision 1.34
diff -u -p -r1.34 sys_socket.c
--- kern/sys_socket.c 14 Nov 2017 16:01:55 -0000 1.34
+++ kern/sys_socket.c 4 Dec 2017 14:20:55 -0000
@@ -166,11 +166,11 @@ soo_poll(struct file *fp, int events, st
  if (revents == 0) {
  if (events & (POLLIN | POLLPRI | POLLRDNORM | POLLRDBAND)) {
  selrecord(p, &so->so_rcv.sb_sel);
- so->so_rcv.sb_flagsintr |= SB_SEL;
+ so->so_rcv.sb_flags |= SB_SEL;
  }
  if (events & (POLLOUT | POLLWRNORM)) {
  selrecord(p, &so->so_snd.sb_sel);
- so->so_snd.sb_flagsintr |= SB_SEL;
+ so->so_snd.sb_flags |= SB_SEL;
  }
  }
  sounlock(s);
Index: kern/uipc_socket2.c
===================================================================
RCS file: /cvs/src/sys/kern/uipc_socket2.c,v
retrieving revision 1.87
diff -u -p -r1.87 uipc_socket2.c
--- kern/uipc_socket2.c 23 Nov 2017 13:42:53 -0000 1.87
+++ kern/uipc_socket2.c 4 Dec 2017 14:22:21 -0000
@@ -329,12 +329,12 @@ sosleep(struct socket *so, void *ident,
 int
 sbwait(struct socket *so, struct sockbuf *sb)
 {
+ int prio = (sb->sb_flags & SB_NOINTR) ? PSOCK : PSOCK | PCATCH;
+
  soassertlocked(so);
 
- sb->sb_flagsintr |= SB_WAIT;
- return (sosleep(so, &sb->sb_cc,
-    (sb->sb_flags & SB_NOINTR) ? PSOCK : PSOCK | PCATCH, "netio",
-    sb->sb_timeo));
+ sb->sb_flags |= SB_WAIT;
+ return (sosleep(so, &sb->sb_cc, prio, "netio", sb->sb_timeo));
 }
 
 int
@@ -381,17 +381,18 @@ sbunlock(struct socket *so, struct sockb
 void
 sowakeup(struct socket *so, struct sockbuf *sb)
 {
- KERNEL_ASSERT_LOCKED();
  soassertlocked(so);
 
- selwakeup(&sb->sb_sel);
- sb->sb_flagsintr &= ~SB_SEL;
- if (sb->sb_flagsintr & SB_WAIT) {
- sb->sb_flagsintr &= ~SB_WAIT;
+ sb->sb_flags &= ~SB_SEL;
+ if (sb->sb_flags & SB_WAIT) {
+ sb->sb_flags &= ~SB_WAIT;
  wakeup(&sb->sb_cc);
  }
+ KERNEL_LOCK();
  if (so->so_state & SS_ASYNC)
  csignal(so->so_pgid, SIGIO, so->so_siguid, so->so_sigeuid);
+ selwakeup(&sb->sb_sel);
+ KERNEL_UNLOCK();
 }
 
 /*
Index: miscfs/fifofs/fifo_vnops.c
===================================================================
RCS file: /cvs/src/sys/miscfs/fifofs/fifo_vnops.c,v
retrieving revision 1.59
diff -u -p -r1.59 fifo_vnops.c
--- miscfs/fifofs/fifo_vnops.c 4 Nov 2017 14:13:53 -0000 1.59
+++ miscfs/fifofs/fifo_vnops.c 4 Dec 2017 14:24:39 -0000
@@ -307,10 +307,12 @@ fifo_poll(void *v)
  struct socket *wso = ap->a_vp->v_fifoinfo->fi_writesock;
  int events = 0;
  int revents = 0;
+ int s;
 
  /*
  * FIFOs don't support out-of-band or high priority data.
  */
+ s = solock(rso);
  if (ap->a_fflag & FREAD)
  events |= ap->a_events & (POLLIN | POLLRDNORM);
  if (ap->a_fflag & FWRITE)
@@ -340,6 +342,7 @@ fifo_poll(void *v)
  wso->so_snd.sb_flagsintr |= SB_SEL;
  }
  }
+ sounlock(s);
  return (revents);
 }
 
@@ -526,7 +529,7 @@ fifo_kqfilter(void *v)
  ap->a_kn->kn_hook = so;
 
  SLIST_INSERT_HEAD(&sb->sb_sel.si_note, ap->a_kn, kn_selnext);
- sb->sb_flags |= SB_KNOTE;
+ sb->sb_flagsintr |= SB_KNOTE;
 
  return (0);
 }
@@ -538,7 +541,7 @@ filt_fifordetach(struct knote *kn)
 
  SLIST_REMOVE(&so->so_rcv.sb_sel.si_note, kn, knote, kn_selnext);
  if (SLIST_EMPTY(&so->so_rcv.sb_sel.si_note))
- so->so_rcv.sb_flags &= ~SB_KNOTE;
+ so->so_rcv.sb_flagsintr &= ~SB_KNOTE;
 }
 
 int

Reply | Threaded
Open this post in threaded view
|

Re: sb_flags vs sb_flagsintr

Alexander Bluhm
On Mon, Dec 04, 2017 at 03:33:56PM +0100, Martin Pieuchot wrote:
> Diff below change the usage of `sb_flags' and `sb_flagsintr'.  The
> former will be protected by the socket lock while the latter will
> be using atomic operations.

I like this plan.

> @@ -381,17 +381,18 @@ sbunlock(struct socket *so, struct sockb
>  void
>  sowakeup(struct socket *so, struct sockbuf *sb)
>  {
> - KERNEL_ASSERT_LOCKED();
>   soassertlocked(so);
>  
> - selwakeup(&sb->sb_sel);
> - sb->sb_flagsintr &= ~SB_SEL;
> - if (sb->sb_flagsintr & SB_WAIT) {
> - sb->sb_flagsintr &= ~SB_WAIT;
> + sb->sb_flags &= ~SB_SEL;
> + if (sb->sb_flags & SB_WAIT) {
> + sb->sb_flags &= ~SB_WAIT;
>   wakeup(&sb->sb_cc);
>   }
> + KERNEL_LOCK();
>   if (so->so_state & SS_ASYNC)
>   csignal(so->so_pgid, SIGIO, so->so_siguid, so->so_sigeuid);
> + selwakeup(&sb->sb_sel);
> + KERNEL_UNLOCK();
>  }

Why do you move the selwakeup() after the csignal() ?

> @@ -340,6 +342,7 @@ fifo_poll(void *v)
>   wso->so_snd.sb_flagsintr |= SB_SEL;
>   }
>   }
> + sounlock(s);
>   return (revents);
>  }

Here you missed to convert sb_flagsintr |= SB_SEL to sb_flags.

And in filt_fifowdetach() is still a sb_flags &= ~SB_KNOTE.

bluhm

Reply | Threaded
Open this post in threaded view
|

Re: sb_flags vs sb_flagsintr

Martin Pieuchot
On 05/12/17(Tue) 14:37, Alexander Bluhm wrote:

> On Mon, Dec 04, 2017 at 03:33:56PM +0100, Martin Pieuchot wrote:
> > Diff below change the usage of `sb_flags' and `sb_flagsintr'.  The
> > former will be protected by the socket lock while the latter will
> > be using atomic operations.
>
> I like this plan.
>
> > @@ -381,17 +381,18 @@ sbunlock(struct socket *so, struct sockb
> >  void
> >  sowakeup(struct socket *so, struct sockbuf *sb)
> >  {
> > - KERNEL_ASSERT_LOCKED();
> >   soassertlocked(so);
> >  
> > - selwakeup(&sb->sb_sel);
> > - sb->sb_flagsintr &= ~SB_SEL;
> > - if (sb->sb_flagsintr & SB_WAIT) {
> > - sb->sb_flagsintr &= ~SB_WAIT;
> > + sb->sb_flags &= ~SB_SEL;
> > + if (sb->sb_flags & SB_WAIT) {
> > + sb->sb_flags &= ~SB_WAIT;
> >   wakeup(&sb->sb_cc);
> >   }
> > + KERNEL_LOCK();
> >   if (so->so_state & SS_ASYNC)
> >   csignal(so->so_pgid, SIGIO, so->so_siguid, so->so_sigeuid);
> > + selwakeup(&sb->sb_sel);
> > + KERNEL_UNLOCK();
> >  }
>
> Why do you move the selwakeup() after the csignal() ?

To make it similar to sohasoutofband().

>
> > @@ -340,6 +342,7 @@ fifo_poll(void *v)
> >   wso->so_snd.sb_flagsintr |= SB_SEL;
> >   }
> >   }
> > + sounlock(s);
> >   return (revents);
> >  }
>
> Here you missed to convert sb_flagsintr |= SB_SEL to sb_flags.
>
> And in filt_fifowdetach() is still a sb_flags &= ~SB_KNOTE.

Right, updated diff below.

Index: kern/uipc_socket.c
===================================================================
RCS file: /cvs/src/sys/kern/uipc_socket.c,v
retrieving revision 1.209
diff -u -p -r1.209 uipc_socket.c
--- kern/uipc_socket.c 23 Nov 2017 13:45:46 -0000 1.209
+++ kern/uipc_socket.c 4 Dec 2017 14:26:24 -0000
@@ -1054,9 +1054,9 @@ sorflush(struct socket *so)
  aso.so_rcv = *sb;
  memset(sb, 0, sizeof (*sb));
  /* XXX - the memset stomps all over so_rcv */
- if (aso.so_rcv.sb_flags & SB_KNOTE) {
+ if (aso.so_rcv.sb_flagsintr & SB_KNOTE) {
  sb->sb_sel.si_note = aso.so_rcv.sb_sel.si_note;
- sb->sb_flags = SB_KNOTE;
+ sb->sb_flagsintr = SB_KNOTE;
  }
  if (pr->pr_flags & PR_RIGHTS && pr->pr_domain->dom_dispose)
  (*pr->pr_domain->dom_dispose)(aso.so_rcv.sb_mb);
@@ -1178,8 +1178,8 @@ sosplice(struct socket *so, int fd, off_
  * we sleep, the socket buffers are not marked as spliced yet.
  */
  if (somove(so, M_WAIT)) {
- so->so_rcv.sb_flagsintr |= SB_SPLICE;
- sosp->so_snd.sb_flagsintr |= SB_SPLICE;
+ so->so_rcv.sb_flags |= SB_SPLICE;
+ sosp->so_snd.sb_flags |= SB_SPLICE;
  }
 
  release:
@@ -1196,8 +1196,8 @@ sounsplice(struct socket *so, struct soc
 
  task_del(sosplice_taskq, &so->so_splicetask);
  timeout_del(&so->so_idleto);
- sosp->so_snd.sb_flagsintr &= ~SB_SPLICE;
- so->so_rcv.sb_flagsintr &= ~SB_SPLICE;
+ sosp->so_snd.sb_flags &= ~SB_SPLICE;
+ so->so_rcv.sb_flags &= ~SB_SPLICE;
  so->so_sp->ssp_socket = sosp->so_sp->ssp_soback = NULL;
  if (wakeup && soreadable(so))
  sorwakeup(so);
@@ -1210,7 +1210,7 @@ soidle(void *arg)
  int s;
 
  s = solock(so);
- if (so->so_rcv.sb_flagsintr & SB_SPLICE) {
+ if (so->so_rcv.sb_flags & SB_SPLICE) {
  so->so_error = ETIMEDOUT;
  sounsplice(so, so->so_sp->ssp_socket, 1);
  }
@@ -1224,7 +1224,7 @@ sotask(void *arg)
  int s;
 
  s = solock(so);
- if (so->so_rcv.sb_flagsintr & SB_SPLICE) {
+ if (so->so_rcv.sb_flags & SB_SPLICE) {
  /*
  * We may not sleep here as sofree() and unsplice() may be
  * called from softnet interrupt context.  This would remove
@@ -1527,7 +1527,7 @@ sorwakeup(struct socket *so)
  soassertlocked(so);
 
 #ifdef SOCKET_SPLICE
- if (so->so_rcv.sb_flagsintr & SB_SPLICE) {
+ if (so->so_rcv.sb_flags & SB_SPLICE) {
  /*
  * TCP has a sendbuffer that can handle multiple packets
  * at once.  So queue the stream a bit to accumulate data.
@@ -1555,7 +1555,7 @@ sowwakeup(struct socket *so)
  soassertlocked(so);
 
 #ifdef SOCKET_SPLICE
- if (so->so_snd.sb_flagsintr & SB_SPLICE)
+ if (so->so_snd.sb_flags & SB_SPLICE)
  task_add(sosplice_taskq, &so->so_sp->ssp_soback->so_splicetask);
 #endif
  sowakeup(so, &so->so_snd);
@@ -1871,9 +1871,10 @@ sogetopt(struct socket *so, int level, i
 void
 sohasoutofband(struct socket *so)
 {
- KERNEL_ASSERT_LOCKED();
+ KERNEL_LOCK();
  csignal(so->so_pgid, SIGURG, so->so_siguid, so->so_sigeuid);
  selwakeup(&so->so_rcv.sb_sel);
+ KERNEL_UNLOCK();
 }
 
 int
@@ -1901,7 +1902,7 @@ soo_kqfilter(struct file *fp, struct kno
  }
 
  SLIST_INSERT_HEAD(&sb->sb_sel.si_note, kn, kn_selnext);
- sb->sb_flags |= SB_KNOTE;
+ sb->sb_flagsintr |= SB_KNOTE;
 
  return (0);
 }
@@ -1915,7 +1916,7 @@ filt_sordetach(struct knote *kn)
 
  SLIST_REMOVE(&so->so_rcv.sb_sel.si_note, kn, knote, kn_selnext);
  if (SLIST_EMPTY(&so->so_rcv.sb_sel.si_note))
- so->so_rcv.sb_flags &= ~SB_KNOTE;
+ so->so_rcv.sb_flagsintr &= ~SB_KNOTE;
 }
 
 int
@@ -1958,7 +1959,7 @@ filt_sowdetach(struct knote *kn)
 
  SLIST_REMOVE(&so->so_snd.sb_sel.si_note, kn, knote, kn_selnext);
  if (SLIST_EMPTY(&so->so_snd.sb_sel.si_note))
- so->so_snd.sb_flags &= ~SB_KNOTE;
+ so->so_snd.sb_flagsintr &= ~SB_KNOTE;
 }
 
 int
Index: kern/sys_socket.c
===================================================================
RCS file: /cvs/src/sys/kern/sys_socket.c,v
retrieving revision 1.34
diff -u -p -r1.34 sys_socket.c
--- kern/sys_socket.c 14 Nov 2017 16:01:55 -0000 1.34
+++ kern/sys_socket.c 4 Dec 2017 14:20:55 -0000
@@ -166,11 +166,11 @@ soo_poll(struct file *fp, int events, st
  if (revents == 0) {
  if (events & (POLLIN | POLLPRI | POLLRDNORM | POLLRDBAND)) {
  selrecord(p, &so->so_rcv.sb_sel);
- so->so_rcv.sb_flagsintr |= SB_SEL;
+ so->so_rcv.sb_flags |= SB_SEL;
  }
  if (events & (POLLOUT | POLLWRNORM)) {
  selrecord(p, &so->so_snd.sb_sel);
- so->so_snd.sb_flagsintr |= SB_SEL;
+ so->so_snd.sb_flags |= SB_SEL;
  }
  }
  sounlock(s);
Index: kern/uipc_socket2.c
===================================================================
RCS file: /cvs/src/sys/kern/uipc_socket2.c,v
retrieving revision 1.87
diff -u -p -r1.87 uipc_socket2.c
--- kern/uipc_socket2.c 23 Nov 2017 13:42:53 -0000 1.87
+++ kern/uipc_socket2.c 4 Dec 2017 14:22:21 -0000
@@ -329,12 +329,12 @@ sosleep(struct socket *so, void *ident,
 int
 sbwait(struct socket *so, struct sockbuf *sb)
 {
+ int prio = (sb->sb_flags & SB_NOINTR) ? PSOCK : PSOCK | PCATCH;
+
  soassertlocked(so);
 
- sb->sb_flagsintr |= SB_WAIT;
- return (sosleep(so, &sb->sb_cc,
-    (sb->sb_flags & SB_NOINTR) ? PSOCK : PSOCK | PCATCH, "netio",
-    sb->sb_timeo));
+ sb->sb_flags |= SB_WAIT;
+ return (sosleep(so, &sb->sb_cc, prio, "netio", sb->sb_timeo));
 }
 
 int
@@ -381,17 +381,18 @@ sbunlock(struct socket *so, struct sockb
 void
 sowakeup(struct socket *so, struct sockbuf *sb)
 {
- KERNEL_ASSERT_LOCKED();
  soassertlocked(so);
 
- selwakeup(&sb->sb_sel);
- sb->sb_flagsintr &= ~SB_SEL;
- if (sb->sb_flagsintr & SB_WAIT) {
- sb->sb_flagsintr &= ~SB_WAIT;
+ sb->sb_flags &= ~SB_SEL;
+ if (sb->sb_flags & SB_WAIT) {
+ sb->sb_flags &= ~SB_WAIT;
  wakeup(&sb->sb_cc);
  }
+ KERNEL_LOCK();
  if (so->so_state & SS_ASYNC)
  csignal(so->so_pgid, SIGIO, so->so_siguid, so->so_sigeuid);
+ selwakeup(&sb->sb_sel);
+ KERNEL_UNLOCK();
 }
 
 /*
Index: miscfs/fifofs/fifo_vnops.c
===================================================================
RCS file: /cvs/src/sys/miscfs/fifofs/fifo_vnops.c,v
retrieving revision 1.59
diff -u -p -r1.59 fifo_vnops.c
--- miscfs/fifofs/fifo_vnops.c 4 Nov 2017 14:13:53 -0000 1.59
+++ miscfs/fifofs/fifo_vnops.c 7 Dec 2017 12:31:07 -0000
@@ -307,10 +307,12 @@ fifo_poll(void *v)
  struct socket *wso = ap->a_vp->v_fifoinfo->fi_writesock;
  int events = 0;
  int revents = 0;
+ int s;
 
  /*
  * FIFOs don't support out-of-band or high priority data.
  */
+ s = solock(rso);
  if (ap->a_fflag & FREAD)
  events |= ap->a_events & (POLLIN | POLLRDNORM);
  if (ap->a_fflag & FWRITE)
@@ -333,13 +335,14 @@ fifo_poll(void *v)
  events = POLLIN;
  if (events & (POLLIN | POLLRDNORM)) {
  selrecord(ap->a_p, &rso->so_rcv.sb_sel);
- rso->so_rcv.sb_flagsintr |= SB_SEL;
+ rso->so_rcv.sb_flags |= SB_SEL;
  }
  if (events & (POLLOUT | POLLWRNORM)) {
  selrecord(ap->a_p, &wso->so_snd.sb_sel);
- wso->so_snd.sb_flagsintr |= SB_SEL;
+ wso->so_snd.sb_flags |= SB_SEL;
  }
  }
+ sounlock(s);
  return (revents);
 }
 
@@ -526,7 +529,7 @@ fifo_kqfilter(void *v)
  ap->a_kn->kn_hook = so;
 
  SLIST_INSERT_HEAD(&sb->sb_sel.si_note, ap->a_kn, kn_selnext);
- sb->sb_flags |= SB_KNOTE;
+ sb->sb_flagsintr |= SB_KNOTE;
 
  return (0);
 }
@@ -538,7 +541,7 @@ filt_fifordetach(struct knote *kn)
 
  SLIST_REMOVE(&so->so_rcv.sb_sel.si_note, kn, knote, kn_selnext);
  if (SLIST_EMPTY(&so->so_rcv.sb_sel.si_note))
- so->so_rcv.sb_flags &= ~SB_KNOTE;
+ so->so_rcv.sb_flagsintr &= ~SB_KNOTE;
 }
 
 int
@@ -570,7 +573,7 @@ filt_fifowdetach(struct knote *kn)
 
  SLIST_REMOVE(&so->so_snd.sb_sel.si_note, kn, knote, kn_selnext);
  if (SLIST_EMPTY(&so->so_snd.sb_sel.si_note))
- so->so_snd.sb_flags &= ~SB_KNOTE;
+ so->so_snd.sb_flagsintr &= ~SB_KNOTE;
 }
 
 int

Reply | Threaded
Open this post in threaded view
|

Re: sb_flags vs sb_flagsintr

Alexander Bluhm
On Thu, Dec 07, 2017 at 01:37:15PM +0100, Martin Pieuchot wrote:
> Right, updated diff below.

OK bluhm@

> Index: kern/uipc_socket.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/uipc_socket.c,v
> retrieving revision 1.209
> diff -u -p -r1.209 uipc_socket.c
> --- kern/uipc_socket.c 23 Nov 2017 13:45:46 -0000 1.209
> +++ kern/uipc_socket.c 4 Dec 2017 14:26:24 -0000
> @@ -1054,9 +1054,9 @@ sorflush(struct socket *so)
>   aso.so_rcv = *sb;
>   memset(sb, 0, sizeof (*sb));
>   /* XXX - the memset stomps all over so_rcv */
> - if (aso.so_rcv.sb_flags & SB_KNOTE) {
> + if (aso.so_rcv.sb_flagsintr & SB_KNOTE) {
>   sb->sb_sel.si_note = aso.so_rcv.sb_sel.si_note;
> - sb->sb_flags = SB_KNOTE;
> + sb->sb_flagsintr = SB_KNOTE;
>   }
>   if (pr->pr_flags & PR_RIGHTS && pr->pr_domain->dom_dispose)
>   (*pr->pr_domain->dom_dispose)(aso.so_rcv.sb_mb);
> @@ -1178,8 +1178,8 @@ sosplice(struct socket *so, int fd, off_
>   * we sleep, the socket buffers are not marked as spliced yet.
>   */
>   if (somove(so, M_WAIT)) {
> - so->so_rcv.sb_flagsintr |= SB_SPLICE;
> - sosp->so_snd.sb_flagsintr |= SB_SPLICE;
> + so->so_rcv.sb_flags |= SB_SPLICE;
> + sosp->so_snd.sb_flags |= SB_SPLICE;
>   }
>  
>   release:
> @@ -1196,8 +1196,8 @@ sounsplice(struct socket *so, struct soc
>  
>   task_del(sosplice_taskq, &so->so_splicetask);
>   timeout_del(&so->so_idleto);
> - sosp->so_snd.sb_flagsintr &= ~SB_SPLICE;
> - so->so_rcv.sb_flagsintr &= ~SB_SPLICE;
> + sosp->so_snd.sb_flags &= ~SB_SPLICE;
> + so->so_rcv.sb_flags &= ~SB_SPLICE;
>   so->so_sp->ssp_socket = sosp->so_sp->ssp_soback = NULL;
>   if (wakeup && soreadable(so))
>   sorwakeup(so);
> @@ -1210,7 +1210,7 @@ soidle(void *arg)
>   int s;
>  
>   s = solock(so);
> - if (so->so_rcv.sb_flagsintr & SB_SPLICE) {
> + if (so->so_rcv.sb_flags & SB_SPLICE) {
>   so->so_error = ETIMEDOUT;
>   sounsplice(so, so->so_sp->ssp_socket, 1);
>   }
> @@ -1224,7 +1224,7 @@ sotask(void *arg)
>   int s;
>  
>   s = solock(so);
> - if (so->so_rcv.sb_flagsintr & SB_SPLICE) {
> + if (so->so_rcv.sb_flags & SB_SPLICE) {
>   /*
>   * We may not sleep here as sofree() and unsplice() may be
>   * called from softnet interrupt context.  This would remove
> @@ -1527,7 +1527,7 @@ sorwakeup(struct socket *so)
>   soassertlocked(so);
>  
>  #ifdef SOCKET_SPLICE
> - if (so->so_rcv.sb_flagsintr & SB_SPLICE) {
> + if (so->so_rcv.sb_flags & SB_SPLICE) {
>   /*
>   * TCP has a sendbuffer that can handle multiple packets
>   * at once.  So queue the stream a bit to accumulate data.
> @@ -1555,7 +1555,7 @@ sowwakeup(struct socket *so)
>   soassertlocked(so);
>  
>  #ifdef SOCKET_SPLICE
> - if (so->so_snd.sb_flagsintr & SB_SPLICE)
> + if (so->so_snd.sb_flags & SB_SPLICE)
>   task_add(sosplice_taskq, &so->so_sp->ssp_soback->so_splicetask);
>  #endif
>   sowakeup(so, &so->so_snd);
> @@ -1871,9 +1871,10 @@ sogetopt(struct socket *so, int level, i
>  void
>  sohasoutofband(struct socket *so)
>  {
> - KERNEL_ASSERT_LOCKED();
> + KERNEL_LOCK();
>   csignal(so->so_pgid, SIGURG, so->so_siguid, so->so_sigeuid);
>   selwakeup(&so->so_rcv.sb_sel);
> + KERNEL_UNLOCK();
>  }
>  
>  int
> @@ -1901,7 +1902,7 @@ soo_kqfilter(struct file *fp, struct kno
>   }
>  
>   SLIST_INSERT_HEAD(&sb->sb_sel.si_note, kn, kn_selnext);
> - sb->sb_flags |= SB_KNOTE;
> + sb->sb_flagsintr |= SB_KNOTE;
>  
>   return (0);
>  }
> @@ -1915,7 +1916,7 @@ filt_sordetach(struct knote *kn)
>  
>   SLIST_REMOVE(&so->so_rcv.sb_sel.si_note, kn, knote, kn_selnext);
>   if (SLIST_EMPTY(&so->so_rcv.sb_sel.si_note))
> - so->so_rcv.sb_flags &= ~SB_KNOTE;
> + so->so_rcv.sb_flagsintr &= ~SB_KNOTE;
>  }
>  
>  int
> @@ -1958,7 +1959,7 @@ filt_sowdetach(struct knote *kn)
>  
>   SLIST_REMOVE(&so->so_snd.sb_sel.si_note, kn, knote, kn_selnext);
>   if (SLIST_EMPTY(&so->so_snd.sb_sel.si_note))
> - so->so_snd.sb_flags &= ~SB_KNOTE;
> + so->so_snd.sb_flagsintr &= ~SB_KNOTE;
>  }
>  
>  int
> Index: kern/sys_socket.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/sys_socket.c,v
> retrieving revision 1.34
> diff -u -p -r1.34 sys_socket.c
> --- kern/sys_socket.c 14 Nov 2017 16:01:55 -0000 1.34
> +++ kern/sys_socket.c 4 Dec 2017 14:20:55 -0000
> @@ -166,11 +166,11 @@ soo_poll(struct file *fp, int events, st
>   if (revents == 0) {
>   if (events & (POLLIN | POLLPRI | POLLRDNORM | POLLRDBAND)) {
>   selrecord(p, &so->so_rcv.sb_sel);
> - so->so_rcv.sb_flagsintr |= SB_SEL;
> + so->so_rcv.sb_flags |= SB_SEL;
>   }
>   if (events & (POLLOUT | POLLWRNORM)) {
>   selrecord(p, &so->so_snd.sb_sel);
> - so->so_snd.sb_flagsintr |= SB_SEL;
> + so->so_snd.sb_flags |= SB_SEL;
>   }
>   }
>   sounlock(s);
> Index: kern/uipc_socket2.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/uipc_socket2.c,v
> retrieving revision 1.87
> diff -u -p -r1.87 uipc_socket2.c
> --- kern/uipc_socket2.c 23 Nov 2017 13:42:53 -0000 1.87
> +++ kern/uipc_socket2.c 4 Dec 2017 14:22:21 -0000
> @@ -329,12 +329,12 @@ sosleep(struct socket *so, void *ident,
>  int
>  sbwait(struct socket *so, struct sockbuf *sb)
>  {
> + int prio = (sb->sb_flags & SB_NOINTR) ? PSOCK : PSOCK | PCATCH;
> +
>   soassertlocked(so);
>  
> - sb->sb_flagsintr |= SB_WAIT;
> - return (sosleep(so, &sb->sb_cc,
> -    (sb->sb_flags & SB_NOINTR) ? PSOCK : PSOCK | PCATCH, "netio",
> -    sb->sb_timeo));
> + sb->sb_flags |= SB_WAIT;
> + return (sosleep(so, &sb->sb_cc, prio, "netio", sb->sb_timeo));
>  }
>  
>  int
> @@ -381,17 +381,18 @@ sbunlock(struct socket *so, struct sockb
>  void
>  sowakeup(struct socket *so, struct sockbuf *sb)
>  {
> - KERNEL_ASSERT_LOCKED();
>   soassertlocked(so);
>  
> - selwakeup(&sb->sb_sel);
> - sb->sb_flagsintr &= ~SB_SEL;
> - if (sb->sb_flagsintr & SB_WAIT) {
> - sb->sb_flagsintr &= ~SB_WAIT;
> + sb->sb_flags &= ~SB_SEL;
> + if (sb->sb_flags & SB_WAIT) {
> + sb->sb_flags &= ~SB_WAIT;
>   wakeup(&sb->sb_cc);
>   }
> + KERNEL_LOCK();
>   if (so->so_state & SS_ASYNC)
>   csignal(so->so_pgid, SIGIO, so->so_siguid, so->so_sigeuid);
> + selwakeup(&sb->sb_sel);
> + KERNEL_UNLOCK();
>  }
>  
>  /*
> Index: miscfs/fifofs/fifo_vnops.c
> ===================================================================
> RCS file: /cvs/src/sys/miscfs/fifofs/fifo_vnops.c,v
> retrieving revision 1.59
> diff -u -p -r1.59 fifo_vnops.c
> --- miscfs/fifofs/fifo_vnops.c 4 Nov 2017 14:13:53 -0000 1.59
> +++ miscfs/fifofs/fifo_vnops.c 7 Dec 2017 12:31:07 -0000
> @@ -307,10 +307,12 @@ fifo_poll(void *v)
>   struct socket *wso = ap->a_vp->v_fifoinfo->fi_writesock;
>   int events = 0;
>   int revents = 0;
> + int s;
>  
>   /*
>   * FIFOs don't support out-of-band or high priority data.
>   */
> + s = solock(rso);
>   if (ap->a_fflag & FREAD)
>   events |= ap->a_events & (POLLIN | POLLRDNORM);
>   if (ap->a_fflag & FWRITE)
> @@ -333,13 +335,14 @@ fifo_poll(void *v)
>   events = POLLIN;
>   if (events & (POLLIN | POLLRDNORM)) {
>   selrecord(ap->a_p, &rso->so_rcv.sb_sel);
> - rso->so_rcv.sb_flagsintr |= SB_SEL;
> + rso->so_rcv.sb_flags |= SB_SEL;
>   }
>   if (events & (POLLOUT | POLLWRNORM)) {
>   selrecord(ap->a_p, &wso->so_snd.sb_sel);
> - wso->so_snd.sb_flagsintr |= SB_SEL;
> + wso->so_snd.sb_flags |= SB_SEL;
>   }
>   }
> + sounlock(s);
>   return (revents);
>  }
>  
> @@ -526,7 +529,7 @@ fifo_kqfilter(void *v)
>   ap->a_kn->kn_hook = so;
>  
>   SLIST_INSERT_HEAD(&sb->sb_sel.si_note, ap->a_kn, kn_selnext);
> - sb->sb_flags |= SB_KNOTE;
> + sb->sb_flagsintr |= SB_KNOTE;
>  
>   return (0);
>  }
> @@ -538,7 +541,7 @@ filt_fifordetach(struct knote *kn)
>  
>   SLIST_REMOVE(&so->so_rcv.sb_sel.si_note, kn, knote, kn_selnext);
>   if (SLIST_EMPTY(&so->so_rcv.sb_sel.si_note))
> - so->so_rcv.sb_flags &= ~SB_KNOTE;
> + so->so_rcv.sb_flagsintr &= ~SB_KNOTE;
>  }
>  
>  int
> @@ -570,7 +573,7 @@ filt_fifowdetach(struct knote *kn)
>  
>   SLIST_REMOVE(&so->so_snd.sb_sel.si_note, kn, knote, kn_selnext);
>   if (SLIST_EMPTY(&so->so_snd.sb_sel.si_note))
> - so->so_snd.sb_flags &= ~SB_KNOTE;
> + so->so_snd.sb_flagsintr &= ~SB_KNOTE;
>  }
>  
>  int