pipe: revisiting pipe: step 2: pipe_create() / pipe_free()

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

pipe: revisiting pipe: step 2: pipe_create() / pipe_free()

Sebastien Marie-3
Hi,

Next move in revisiting pipe initialization.

After some discussion with mpi@, it seems better to have the whole
`struct pipe' allocation and initialization inside pipe_create()
function, aka moving pool_get() inside pipe_create() instead of
allocating the struct in dopipe() and initialize it in pipe_create().

It makes pipe_create() to return a `struct pipe *' instead of an error
code. The sole possible error was ENOMEM, so returning NULL in such case
is fine.


In the same move, I revisited the freeing of the struct pipe: the
pipeclose() function.

About pipeclose():

- I dislike having both pipe_close() and pipeclose() functions. So I
  renamed pipeclose() to pipe_free(). The function does a bit more that
  freeing resources, as it ensures all customers possibling in wait state
  are aware of the pipe terminaison. but I don't have better name...

- I inversed the if-condition to return early, and move the whole
  if-body in the function. It is more readable, and match what we usually
  do in such case.

This way, pipe_create() and pipe_free() are symetric: allocation in one,
and free in another.

Comments or OK ?
--
Sebastien Marie


Index: sys/kern/sys_pipe.c
===================================================================
--- sys/kern/sys_pipe.c.orig 2019-07-15 11:06:05.015708914 +0200
+++ sys/kern/sys_pipe.c 2019-07-15 11:11:35.667317769 +0200
@@ -97,12 +97,12 @@ static unsigned int amountpipekva;
 struct pool pipe_pool;
 
 int dopipe(struct proc *, int *, int);
-void pipeclose(struct pipe *);
-int pipe_create(struct pipe *);
 int pipelock(struct pipe *);
 void pipeunlock(struct pipe *);
 void pipeselwakeup(struct pipe *);
 
+struct pipe *pipe_create();
+void pipe_free(struct pipe *);
 int pipe_buffer_realloc(struct pipe *, u_int);
 void pipe_buffer_free(struct pipe *);
 
@@ -144,14 +144,11 @@ dopipe(struct proc *p, int *ufds, int fl
 
  cloexec = (flags & O_CLOEXEC) ? UF_EXCLOSE : 0;
 
- rpipe = pool_get(&pipe_pool, PR_WAITOK | PR_ZERO);
- error = pipe_create(rpipe);
- if (error != 0)
- goto free1;
- wpipe = pool_get(&pipe_pool, PR_WAITOK | PR_ZERO);
- error = pipe_create(wpipe);
- if (error != 0)
+ if (((rpipe = pipe_create()) == NULL) ||
+    ((wpipe = pipe_create()) == NULL)) {
+ error = ENOMEM;
  goto free1;
+ }
 
  fdplock(fdp);
 
@@ -202,8 +199,8 @@ free3:
 free2:
  fdpunlock(fdp);
 free1:
- pipeclose(wpipe);
- pipeclose(rpipe);
+ pipe_free(wpipe);
+ pipe_free(rpipe);
  return (error);
 }
 
@@ -247,16 +244,19 @@ pipe_buffer_realloc(struct pipe *cpipe,
 /*
  * initialize and allocate VM and memory for pipe
  */
-int
-pipe_create(struct pipe *cpipe)
+struct pipe *
+pipe_create()
 {
  int error;
-
- sigio_init(&cpipe->pipe_sigio);
+ struct pipe *cpipe = pool_get(&pipe_pool, PR_WAITOK | PR_ZERO);
 
  error = pipe_buffer_realloc(cpipe, PIPE_SIZE);
- if (error != 0)
- return (error);
+ if (error != 0) {
+ pool_put(&pipe_pool, cpipe);
+ return (NULL);
+ }
+
+ sigio_init(&cpipe->pipe_sigio);
 
  getnanotime(&cpipe->pipe_ctime);
  cpipe->pipe_atime = cpipe->pipe_ctime;
@@ -768,7 +768,7 @@ pipe_close(struct file *fp, struct proc
  fp->f_ops = NULL;
  fp->f_data = NULL;
  KERNEL_LOCK();
- pipeclose(cpipe);
+ pipe_free(cpipe);
  KERNEL_UNLOCK();
  return (0);
 }
@@ -799,44 +799,46 @@ pipe_buffer_free(struct pipe *cpipe)
 }
 
 /*
- * shutdown the pipe
+ * shutdown the pipe, and free resources.
  */
 void
-pipeclose(struct pipe *cpipe)
+pipe_free(struct pipe *cpipe)
 {
  struct pipe *ppipe;
- if (cpipe) {
- pipeselwakeup(cpipe);
- sigio_free(&cpipe->pipe_sigio);
 
- /*
- * If the other side is blocked, wake it up saying that
- * we want to close it down.
- */
- cpipe->pipe_state |= PIPE_EOF;
- while (cpipe->pipe_busy) {
- wakeup(cpipe);
- cpipe->pipe_state |= PIPE_WANTD;
- tsleep(cpipe, PRIBIO, "pipecl", 0);
- }
+ if (cpipe == NULL)
+ return;
 
- /*
- * Disconnect from peer
- */
- if ((ppipe = cpipe->pipe_peer) != NULL) {
- pipeselwakeup(ppipe);
+ pipeselwakeup(cpipe);
+ sigio_free(&cpipe->pipe_sigio);
 
- ppipe->pipe_state |= PIPE_EOF;
- wakeup(ppipe);
- ppipe->pipe_peer = NULL;
- }
+ /*
+ * If the other side is blocked, wake it up saying that
+ * we want to close it down.
+ */
+ cpipe->pipe_state |= PIPE_EOF;
+ while (cpipe->pipe_busy) {
+ wakeup(cpipe);
+ cpipe->pipe_state |= PIPE_WANTD;
+ tsleep(cpipe, PRIBIO, "pipecl", 0);
+ }
 
- /*
- * free resources
- */
- pipe_buffer_free(cpipe);
- pool_put(&pipe_pool, cpipe);
+ /*
+ * Disconnect from peer
+ */
+ if ((ppipe = cpipe->pipe_peer) != NULL) {
+ pipeselwakeup(ppipe);
+
+ ppipe->pipe_state |= PIPE_EOF;
+ wakeup(ppipe);
+ ppipe->pipe_peer = NULL;
  }
+
+ /*
+ * free resources
+ */
+ pipe_buffer_free(cpipe);
+ pool_put(&pipe_pool, cpipe);
 }
 
 int

Reply | Threaded
Open this post in threaded view
|

Re: pipe: revisiting pipe: step 2: pipe_create() / pipe_free()

Claudio Jeker
On Mon, Jul 15, 2019 at 12:14:00PM +0200, Sebastien Marie wrote:

> Hi,
>
> Next move in revisiting pipe initialization.
>
> After some discussion with mpi@, it seems better to have the whole
> `struct pipe' allocation and initialization inside pipe_create()
> function, aka moving pool_get() inside pipe_create() instead of
> allocating the struct in dopipe() and initialize it in pipe_create().
>
> It makes pipe_create() to return a `struct pipe *' instead of an error
> code. The sole possible error was ENOMEM, so returning NULL in such case
> is fine.
>
>
> In the same move, I revisited the freeing of the struct pipe: the
> pipeclose() function.
>
> About pipeclose():
>
> - I dislike having both pipe_close() and pipeclose() functions. So I
>   renamed pipeclose() to pipe_free(). The function does a bit more that
>   freeing resources, as it ensures all customers possibling in wait state
>   are aware of the pipe terminaison. but I don't have better name...

I used *_destroy() in similar cases but honestly I don't mind pipe_free().
 

> - I inversed the if-condition to return early, and move the whole
>   if-body in the function. It is more readable, and match what we usually
>   do in such case.
>
> This way, pipe_create() and pipe_free() are symetric: allocation in one,
> and free in another.
>
> Comments or OK ?
> --
> Sebastien Marie
>
>
> Index: sys/kern/sys_pipe.c
> ===================================================================
> --- sys/kern/sys_pipe.c.orig 2019-07-15 11:06:05.015708914 +0200
> +++ sys/kern/sys_pipe.c 2019-07-15 11:11:35.667317769 +0200
> @@ -97,12 +97,12 @@ static unsigned int amountpipekva;
>  struct pool pipe_pool;
>  
>  int dopipe(struct proc *, int *, int);
> -void pipeclose(struct pipe *);
> -int pipe_create(struct pipe *);
>  int pipelock(struct pipe *);
>  void pipeunlock(struct pipe *);
>  void pipeselwakeup(struct pipe *);
>  
> +struct pipe *pipe_create();

Please make this a real void function:
struct pipe *pipe_create(void);

> +void pipe_free(struct pipe *);
>  int pipe_buffer_realloc(struct pipe *, u_int);
>  void pipe_buffer_free(struct pipe *);
>  
> @@ -144,14 +144,11 @@ dopipe(struct proc *p, int *ufds, int fl
>  
>   cloexec = (flags & O_CLOEXEC) ? UF_EXCLOSE : 0;
>  
> - rpipe = pool_get(&pipe_pool, PR_WAITOK | PR_ZERO);
> - error = pipe_create(rpipe);
> - if (error != 0)
> - goto free1;
> - wpipe = pool_get(&pipe_pool, PR_WAITOK | PR_ZERO);
> - error = pipe_create(wpipe);
> - if (error != 0)
> + if (((rpipe = pipe_create()) == NULL) ||
> +    ((wpipe = pipe_create()) == NULL)) {
> + error = ENOMEM;
>   goto free1;
> + }
>  
>   fdplock(fdp);
>  
> @@ -202,8 +199,8 @@ free3:
>  free2:
>   fdpunlock(fdp);
>  free1:
> - pipeclose(wpipe);
> - pipeclose(rpipe);
> + pipe_free(wpipe);
> + pipe_free(rpipe);
>   return (error);
>  }
>  
> @@ -247,16 +244,19 @@ pipe_buffer_realloc(struct pipe *cpipe,
>  /*
>   * initialize and allocate VM and memory for pipe
>   */
> -int
> -pipe_create(struct pipe *cpipe)
> +struct pipe *
> +pipe_create()

again make this really not take any arguments by using
        pipe_create(void)

>  {
>   int error;
> -
> - sigio_init(&cpipe->pipe_sigio);
> + struct pipe *cpipe = pool_get(&pipe_pool, PR_WAITOK | PR_ZERO);

In general complex functions should not be used in initializers.
So maybe make this:

        struct pipe *cpipe;

        cpipe = pool_get(&pipe_pool, PR_WAITOK | PR_ZERO);
 

>   error = pipe_buffer_realloc(cpipe, PIPE_SIZE);
> - if (error != 0)
> - return (error);
> + if (error != 0) {
> + pool_put(&pipe_pool, cpipe);
> + return (NULL);
> + }
> +
> + sigio_init(&cpipe->pipe_sigio);
>  
>   getnanotime(&cpipe->pipe_ctime);
>   cpipe->pipe_atime = cpipe->pipe_ctime;
> @@ -768,7 +768,7 @@ pipe_close(struct file *fp, struct proc
>   fp->f_ops = NULL;
>   fp->f_data = NULL;
>   KERNEL_LOCK();
> - pipeclose(cpipe);
> + pipe_free(cpipe);
>   KERNEL_UNLOCK();
>   return (0);
>  }
> @@ -799,44 +799,46 @@ pipe_buffer_free(struct pipe *cpipe)
>  }
>  
>  /*
> - * shutdown the pipe
> + * shutdown the pipe, and free resources.
>   */
>  void
> -pipeclose(struct pipe *cpipe)
> +pipe_free(struct pipe *cpipe)
>  {
>   struct pipe *ppipe;
> - if (cpipe) {
> - pipeselwakeup(cpipe);
> - sigio_free(&cpipe->pipe_sigio);
>  
> - /*
> - * If the other side is blocked, wake it up saying that
> - * we want to close it down.
> - */
> - cpipe->pipe_state |= PIPE_EOF;
> - while (cpipe->pipe_busy) {
> - wakeup(cpipe);
> - cpipe->pipe_state |= PIPE_WANTD;
> - tsleep(cpipe, PRIBIO, "pipecl", 0);
> - }
> + if (cpipe == NULL)
> + return;
>  
> - /*
> - * Disconnect from peer
> - */
> - if ((ppipe = cpipe->pipe_peer) != NULL) {
> - pipeselwakeup(ppipe);
> + pipeselwakeup(cpipe);
> + sigio_free(&cpipe->pipe_sigio);
>  
> - ppipe->pipe_state |= PIPE_EOF;
> - wakeup(ppipe);
> - ppipe->pipe_peer = NULL;
> - }
> + /*
> + * If the other side is blocked, wake it up saying that
> + * we want to close it down.
> + */
> + cpipe->pipe_state |= PIPE_EOF;
> + while (cpipe->pipe_busy) {
> + wakeup(cpipe);
> + cpipe->pipe_state |= PIPE_WANTD;
> + tsleep(cpipe, PRIBIO, "pipecl", 0);
> + }
>  
> - /*
> - * free resources
> - */
> - pipe_buffer_free(cpipe);
> - pool_put(&pipe_pool, cpipe);
> + /*
> + * Disconnect from peer
> + */
> + if ((ppipe = cpipe->pipe_peer) != NULL) {
> + pipeselwakeup(ppipe);
> +
> + ppipe->pipe_state |= PIPE_EOF;
> + wakeup(ppipe);
> + ppipe->pipe_peer = NULL;
>   }
> +
> + /*
> + * free resources
> + */
> + pipe_buffer_free(cpipe);
> + pool_put(&pipe_pool, cpipe);
>  }
>  
>  int
>

Apart from that OK claudio@

--
:wq Claudio

Reply | Threaded
Open this post in threaded view
|

Re: pipe: revisiting pipe: step 2: pipe_create() / pipe_free()

Sebastien Marie-3
In reply to this post by Sebastien Marie-3
On Mon, Jul 15, 2019 at 12:14:00PM +0200, Sebastien Marie wrote:

> Hi,
>
> Next move in revisiting pipe initialization.
>
> After some discussion with mpi@, it seems better to have the whole
> `struct pipe' allocation and initialization inside pipe_create()
> function, aka moving pool_get() inside pipe_create() instead of
> allocating the struct in dopipe() and initialize it in pipe_create().
>
> It makes pipe_create() to return a `struct pipe *' instead of an error
> code. The sole possible error was ENOMEM, so returning NULL in such case
> is fine.
>
>
> In the same move, I revisited the freeing of the struct pipe: the
> pipeclose() function.
>
> About pipeclose():
>
> - I dislike having both pipe_close() and pipeclose() functions. So I
>   renamed pipeclose() to pipe_free(). The function does a bit more that
>   freeing resources, as it ensures all customers possibling in wait state
>   are aware of the pipe terminaison. but I don't have better name...
>
> - I inversed the if-condition to return early, and move the whole
>   if-body in the function. It is more readable, and match what we usually
>   do in such case.
>
> This way, pipe_create() and pipe_free() are symetric: allocation in one,
> and free in another.

new diff with changes asked by claudio@. I also used pipe_destroy()
name.

I send a new diff as a little omission was present in pipe_create(): it
still returned 0 on success (instead of the newly allocated pointer).
bad semarie@ that didn't test its own diff after splitting it to make a
small diff.

--
Sebastien Marie


Index: kern/sys_pipe.c
===================================================================
RCS file: /cvs/src/sys/kern/sys_pipe.c,v
retrieving revision 1.94
diff -u -p -r1.94 sys_pipe.c
--- kern/sys_pipe.c 15 Jul 2019 09:05:46 -0000 1.94
+++ kern/sys_pipe.c 15 Jul 2019 16:08:42 -0000
@@ -97,12 +97,12 @@ static unsigned int amountpipekva;
 struct pool pipe_pool;
 
 int dopipe(struct proc *, int *, int);
-void pipeclose(struct pipe *);
-int pipe_create(struct pipe *);
 int pipelock(struct pipe *);
 void pipeunlock(struct pipe *);
 void pipeselwakeup(struct pipe *);
 
+struct pipe *pipe_create(void);
+void pipe_destroy(struct pipe *);
 int pipe_buffer_realloc(struct pipe *, u_int);
 void pipe_buffer_free(struct pipe *);
 
@@ -144,14 +144,11 @@ dopipe(struct proc *p, int *ufds, int fl
 
  cloexec = (flags & O_CLOEXEC) ? UF_EXCLOSE : 0;
 
- rpipe = pool_get(&pipe_pool, PR_WAITOK | PR_ZERO);
- error = pipe_create(rpipe);
- if (error != 0)
- goto free1;
- wpipe = pool_get(&pipe_pool, PR_WAITOK | PR_ZERO);
- error = pipe_create(wpipe);
- if (error != 0)
+ if (((rpipe = pipe_create()) == NULL) ||
+    ((wpipe = pipe_create()) == NULL)) {
+ error = ENOMEM;
  goto free1;
+ }
 
  fdplock(fdp);
 
@@ -202,8 +199,8 @@ free3:
 free2:
  fdpunlock(fdp);
 free1:
- pipeclose(wpipe);
- pipeclose(rpipe);
+ pipe_destroy(wpipe);
+ pipe_destroy(rpipe);
  return (error);
 }
 
@@ -247,22 +244,27 @@ pipe_buffer_realloc(struct pipe *cpipe,
 /*
  * initialize and allocate VM and memory for pipe
  */
-int
-pipe_create(struct pipe *cpipe)
+struct pipe *
+pipe_create(void)
 {
+ struct pipe *cpipe;
  int error;
 
- sigio_init(&cpipe->pipe_sigio);
+ cpipe = pool_get(&pipe_pool, PR_WAITOK | PR_ZERO);
 
  error = pipe_buffer_realloc(cpipe, PIPE_SIZE);
- if (error != 0)
- return (error);
+ if (error != 0) {
+ pool_put(&pipe_pool, cpipe);
+ return (NULL);
+ }
+
+ sigio_init(&cpipe->pipe_sigio);
 
  getnanotime(&cpipe->pipe_ctime);
  cpipe->pipe_atime = cpipe->pipe_ctime;
  cpipe->pipe_mtime = cpipe->pipe_ctime;
 
- return (0);
+ return (cpipe);
 }
 
 
@@ -768,7 +770,7 @@ pipe_close(struct file *fp, struct proc
  fp->f_ops = NULL;
  fp->f_data = NULL;
  KERNEL_LOCK();
- pipeclose(cpipe);
+ pipe_destroy(cpipe);
  KERNEL_UNLOCK();
  return (0);
 }
@@ -799,44 +801,46 @@ pipe_buffer_free(struct pipe *cpipe)
 }
 
 /*
- * shutdown the pipe
+ * shutdown the pipe, and free resources.
  */
 void
-pipeclose(struct pipe *cpipe)
+pipe_destroy(struct pipe *cpipe)
 {
  struct pipe *ppipe;
- if (cpipe) {
- pipeselwakeup(cpipe);
- sigio_free(&cpipe->pipe_sigio);
 
- /*
- * If the other side is blocked, wake it up saying that
- * we want to close it down.
- */
- cpipe->pipe_state |= PIPE_EOF;
- while (cpipe->pipe_busy) {
- wakeup(cpipe);
- cpipe->pipe_state |= PIPE_WANTD;
- tsleep(cpipe, PRIBIO, "pipecl", 0);
- }
+ if (cpipe == NULL)
+ return;
 
- /*
- * Disconnect from peer
- */
- if ((ppipe = cpipe->pipe_peer) != NULL) {
- pipeselwakeup(ppipe);
+ pipeselwakeup(cpipe);
+ sigio_free(&cpipe->pipe_sigio);
 
- ppipe->pipe_state |= PIPE_EOF;
- wakeup(ppipe);
- ppipe->pipe_peer = NULL;
- }
+ /*
+ * If the other side is blocked, wake it up saying that
+ * we want to close it down.
+ */
+ cpipe->pipe_state |= PIPE_EOF;
+ while (cpipe->pipe_busy) {
+ wakeup(cpipe);
+ cpipe->pipe_state |= PIPE_WANTD;
+ tsleep(cpipe, PRIBIO, "pipecl", 0);
+ }
 
- /*
- * free resources
- */
- pipe_buffer_free(cpipe);
- pool_put(&pipe_pool, cpipe);
+ /*
+ * Disconnect from peer
+ */
+ if ((ppipe = cpipe->pipe_peer) != NULL) {
+ pipeselwakeup(ppipe);
+
+ ppipe->pipe_state |= PIPE_EOF;
+ wakeup(ppipe);
+ ppipe->pipe_peer = NULL;
  }
+
+ /*
+ * free resources
+ */
+ pipe_buffer_free(cpipe);
+ pool_put(&pipe_pool, cpipe);
 }
 
 int