Hi,
The following diff revisits pipe initialiation (pipe_create() function)
and buffer management (pipespace() and pipe_free_kmem() functions).
Regarding pipe initialisation, get an already zeroed struct instead of
manually zeroing each struct members.
Rename pipespace() and pipe_free_kmem() to have more explicit name on
what the functions are for: reallocating or freeing the associated
buffer of the pipe. And it shows there are working on the same thing:
pipe_buffer_realloc() and pipe_buffer_free().
In pipe_free_kmem(), I changed the if-condition to return early instead
of having the whole function body inside the if-body.
No functional change intented.
Two KASSERT() added to pipespace(), one for the lock, another for
ensuring the buffer is empty before realloc.
Comments or OK ?
--
Sebastien Marie
Index: kern/sys_pipe.c
===================================================================
RCS file: /cvs/src/sys/kern/sys_pipe.c,v
retrieving revision 1.91
diff -u -p -r1.91 sys_pipe.c
--- kern/sys_pipe.c 13 Jul 2019 06:51:59 -0000 1.91
+++ kern/sys_pipe.c 13 Jul 2019 07:22:54 -0000
@@ -98,12 +98,13 @@ struct pool pipe_pool;
int dopipe(struct proc *, int *, int);
void pipeclose(struct pipe *);
-void pipe_free_kmem(struct pipe *);
int pipe_create(struct pipe *);
int pipelock(struct pipe *);
void pipeunlock(struct pipe *);
void pipeselwakeup(struct pipe *);
-int pipespace(struct pipe *, u_int);
+
+int pipe_buffer_realloc(struct pipe *, u_int);
+void pipe_buffer_free(struct pipe *);
/*
* The pipe system call for the DTYPE_PIPE type of pipes
@@ -143,11 +144,11 @@ dopipe(struct proc *p, int *ufds, int fl
cloexec = (flags & O_CLOEXEC) ? UF_EXCLOSE : 0;
- rpipe = pool_get(&pipe_pool, PR_WAITOK);
+ 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);
+ wpipe = pool_get(&pipe_pool, PR_WAITOK | PR_ZERO);
error = pipe_create(wpipe);
if (error != 0)
goto free1;
@@ -210,24 +211,30 @@ free1:
* If it fails it will return ENOMEM.
*/
int
-pipespace(struct pipe *cpipe, u_int size)
+pipe_buffer_realloc(struct pipe *cpipe, u_int size)
{
caddr_t buffer;
+ /* buffer uninitialized or pipe locked */
+ KASSERT((cpipe->pipe_buffer.buffer == NULL) ||
+ (cpipe->pipe_state & PIPE_LOCK));
+
+ /* buffer should be empty */
+ KASSERT(cpipe->pipe_buffer.cnt == 0);
+
KERNEL_LOCK();
buffer = km_alloc(size, &kv_any, &kp_pageable, &kd_waitok);
KERNEL_UNLOCK();
- if (buffer == NULL) {
+ if (buffer == NULL)
return (ENOMEM);
- }
/* free old resources if we are resizing */
- pipe_free_kmem(cpipe);
+ pipe_buffer_free(cpipe);
+
cpipe->pipe_buffer.buffer = buffer;
cpipe->pipe_buffer.size = size;
cpipe->pipe_buffer.in = 0;
cpipe->pipe_buffer.out = 0;
- cpipe->pipe_buffer.cnt = 0;
atomic_add_int(&amountpipekva, cpipe->pipe_buffer.size);
@@ -242,19 +249,9 @@ pipe_create(struct pipe *cpipe)
{
int error;
- /* so pipe_free_kmem() doesn't follow junk pointer */
- cpipe->pipe_buffer.buffer = NULL;
- /*
- * protect so pipeclose() doesn't follow a junk pointer
- * if pipespace() fails.
- */
- memset(&cpipe->pipe_sel, 0, sizeof(cpipe->pipe_sel));
- cpipe->pipe_state = 0;
- cpipe->pipe_peer = NULL;
- cpipe->pipe_busy = 0;
sigio_init(&cpipe->pipe_sigio);
- error = pipespace(cpipe, PIPE_SIZE);
+ error = pipe_buffer_realloc(cpipe, PIPE_SIZE);
if (error != 0)
return (error);
@@ -461,7 +458,7 @@ pipe_write(struct file *fp, struct uio *
if ((npipe <= LIMITBIGPIPES) &&
(error = pipelock(wpipe)) == 0) {
if ((wpipe->pipe_buffer.cnt != 0) ||
- (pipespace(wpipe, BIG_PIPE_SIZE) != 0))
+ (pipe_buffer_realloc(wpipe, BIG_PIPE_SIZE) != 0))
atomic_dec_int(&nbigpipe);
pipeunlock(wpipe);
} else
@@ -772,20 +769,29 @@ pipe_close(struct file *fp, struct proc
return (0);
}
+/*
+ * Free kva for pipe circular buffer.
+ * No lock check as only called from pipe_buffer_realloc() and pipeclose()
+ */
void
-pipe_free_kmem(struct pipe *cpipe)
+pipe_buffer_free(struct pipe *cpipe)
{
- u_int size = cpipe->pipe_buffer.size;
+ u_int size;
- if (cpipe->pipe_buffer.buffer != NULL) {
- KERNEL_LOCK();
- km_free(cpipe->pipe_buffer.buffer, size, &kv_any, &kp_pageable);
- KERNEL_UNLOCK();
- atomic_sub_int(&amountpipekva, size);
- cpipe->pipe_buffer.buffer = NULL;
- if (size > PIPE_SIZE)
- atomic_dec_int(&nbigpipe);
- }
+ if (cpipe->pipe_buffer.buffer == NULL)
+ return;
+
+ size = cpipe->pipe_buffer.size;
+
+ KERNEL_LOCK();
+ km_free(cpipe->pipe_buffer.buffer, size, &kv_any, &kp_pageable);
+ KERNEL_UNLOCK();
+
+ cpipe->pipe_buffer.buffer = NULL;
+
+ atomic_sub_int(&amountpipekva, size);
+ if (size > PIPE_SIZE)
+ atomic_dec_int(&nbigpipe);
}
/*
@@ -824,7 +830,7 @@ pipeclose(struct pipe *cpipe)
/*
* free resources
*/
- pipe_free_kmem(cpipe);
+ pipe_buffer_free(cpipe);
pool_put(&pipe_pool, cpipe);
}
}
