Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1243264 > unrolled thread

[PATCH 0/6] staging: comedi: fix some minor issues with file poll op

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/6] staging: comedi: fix some minor issues with file poll op Ian Abbott <abbotti@mev.co.uk> - 2015-10-09 13:30 +0200
    [PATCH 5/6] staging: comedi: check command started by file being polled Ian Abbott <abbotti@mev.co.uk> - 2015-10-09 13:30 +0200
    [PATCH 6/6] staging: comedi: don't use mutex when polling file Ian Abbott <abbotti@mev.co.uk> - 2015-10-09 13:30 +0200
      RE: [PATCH 6/6] staging: comedi: don't use mutex when polling file Hartley Sweeten <HartleyS@visionengravers.com> - 2015-10-09 19:30 +0200
    [PATCH 1/6] staging: comedi: don't poll_wait on same subdevice twice Ian Abbott <abbotti@mev.co.uk> - 2015-10-09 13:30 +0200
    RE: [PATCH 0/6] staging: comedi: fix some minor issues with file poll  op Hartley Sweeten <HartleyS@visionengravers.com> - 2015-10-09 19:30 +0200

#1243264 — [PATCH 0/6] staging: comedi: fix some minor issues with file poll op

A few changes for the "poll" file operation to avoid poll-waiting on the
same subdevice for both read and write (patch 1), avoid allocating write
buffer space unnecessarily and possibly inappropriately (patch 4),
consider whether any active commands belong to the current file object
(patch 5), and avoid using the main mutex (for performance reasons)
(patch 6).

1) staging: comedi: don't poll_wait on same subdevice twice
2) staging: comedi: rename comedi_buf_write_n_available
3) staging: comedi: add new comedi_buf_write_n_available()
4) staging: comedi: don't allocate buffer space when polling for write
5) staging: comedi: check command started by file being polled
6) staging: comedi: don't use mutex when polling file

 drivers/staging/comedi/comedi_buf.c      | 19 +++++++++++++------
 drivers/staging/comedi/comedi_fops.c     | 17 +++++++++--------
 drivers/staging/comedi/comedi_internal.h |  1 +
 3 files changed, 23 insertions(+), 14 deletions(-)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1243265 — [PATCH 5/6] staging: comedi: check command started by file being polled

Currently, the "poll" file operation checks if an asynchronous "read"
(or "write" command is active on the "read" (or "write" subdevice, but
does not consider whether the command was started from the file object
being polled.  Since that is the only file object able to read (or
write) data, take it into consideration.

With this change, if no read (or write) command is running on the
subdevice, or it is started by a different file object, the file object
is marked as readable (or writeable) regardless, but the read (or write)
file operation will return an error.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/staging/comedi/comedi_fops.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index 4793e30..07bb197 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2275,7 +2275,7 @@ static unsigned int comedi_poll(struct file *file, poll_table *wait)
 	s_read = s;
 	if (s && s->async) {
 		poll_wait(file, &s->async->wait_head, wait);
-		if (!s->busy || !comedi_is_subdevice_running(s) ||
+		if (s->busy != file || !comedi_is_subdevice_running(s) ||
 		    (s->async->cmd.flags & CMDF_WRITE) ||
 		    comedi_buf_read_n_available(s) > 0)
 			mask |= POLLIN | POLLRDNORM;
@@ -2287,7 +2287,7 @@ static unsigned int comedi_poll(struct file *file, poll_table *wait)
 
 		if (s != s_read)
 			poll_wait(file, &s->async->wait_head, wait);
-		if (!s->busy || !comedi_is_subdevice_running(s) ||
+		if (s->busy != file || !comedi_is_subdevice_running(s) ||
 		    !(s->async->cmd.flags & CMDF_WRITE) ||
 		    comedi_buf_write_n_available(s) >= bps)
 			mask |= POLLOUT | POLLWRNORM;
-- 
2.6.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1243266 — [PATCH 6/6] staging: comedi: don't use mutex when polling file

The main mutex in a comedi device can get held for quite a while when
processing comedi instructions, so for performance reasons, the "read"
and "write" file operations do not use it; they use use the
`attach_lock` rwsemaphore to protect against the comedi device becoming
detached at an inopportune moment.  Do the same for the "poll" file
operation.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/staging/comedi/comedi_fops.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index 07bb197..88e9334 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2264,7 +2264,7 @@ static unsigned int comedi_poll(struct file *file, poll_table *wait)
 	struct comedi_device *dev = cfp->dev;
 	struct comedi_subdevice *s, *s_read;
 
-	mutex_lock(&dev->mutex);
+	down_read(&dev->attach_lock);
 
 	if (!dev->attached) {
 		dev_dbg(dev->class_dev, "no driver attached\n");
@@ -2294,7 +2294,7 @@ static unsigned int comedi_poll(struct file *file, poll_table *wait)
 	}
 
 done:
-	mutex_unlock(&dev->mutex);
+	up_read(&dev->attach_lock);
 	return mask;
 }
 
-- 
2.6.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1243550 — RE: [PATCH 6/6] staging: comedi: don't use mutex when polling file

On Friday, October 09, 2015 4:27 AM, Ian Abbott wrote:
> The main mutex in a comedi device can get held for quite a while when
> processing comedi instructions, so for performance reasons, the "read"
> and "write" file operations do not use it; they use use the
> `attach_lock` rwsemaphore to protect against the comedi device becoming
> detached at an inopportune moment.  Do the same for the "poll" file
> operation.
>
> Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
> ---
>  drivers/staging/comedi/comedi_fops.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
> index 07bb197..88e9334 100644
> --- a/drivers/staging/comedi/comedi_fops.c
> +++ b/drivers/staging/comedi/comedi_fops.c
> @@ -2264,7 +2264,7 @@ static unsigned int comedi_poll(struct file *file, poll_table *wait)
>  	struct comedi_device *dev = cfp->dev;
>  	struct comedi_subdevice *s, *s_read;
>  
> -	mutex_lock(&dev->mutex);
> +	down_read(&dev->attach_lock);
>  
>  	if (!dev->attached) {
>  		dev_dbg(dev->class_dev, "no driver attached\n");
> @@ -2294,7 +2294,7 @@ static unsigned int comedi_poll(struct file *file, poll_table *wait)
>  	}
>  
>  done:
> -	mutex_unlock(&dev->mutex);
> +	up_read(&dev->attach_lock);
>  	return mask;
>  }

Ian,

No issues with this patch, just a comment:

checkpatch.pl reports some issue about the spinlock_t and mutex definitions
in comedidev.h:

CHECK: spinlock_t definition without comment
#177: FILE: drivers/staging/comedi/comedidev.h:177:
+       spinlock_t spin_lock;

CHECK: spinlock_t definition without comment
#540: FILE: drivers/staging/comedi/comedidev.h:540:
+       spinlock_t spinlock;

CHECK: struct mutex definition without comment
#541: FILE: drivers/staging/comedi/comedidev.h:541:
+       struct mutex mutex;

I know these are documented in the docbook comment for the structs but would
you mind adding some comments to the definitions to quiet checkpatch.pl?

Thanks,
Hartley


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1243270 — [PATCH 1/6] staging: comedi: don't poll_wait on same subdevice twice

Comedi subdevices that support asynchronous acquisition commands have a
wait queue head used for blocking reads or writes and for the poll file
operation.  The comedi device may have several subdevices that support
"read" and/or "write" commands, but each open file object has at most
one "read" subdevice and one "write" subdevice.  It's possible (though
rare) for those to be the same subdevice if the subdevice supports
commands in either direction.  In that case, the "poll" file operation
doesn't really need to do a `poll_wait()` on the same subdevice twice.
Although harmless, it wastes a poll table entry.  Check for that, and
avoid it.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/staging/comedi/comedi_fops.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index ef4b58b..9dcd486 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2262,7 +2262,7 @@ static unsigned int comedi_poll(struct file *file, poll_table *wait)
 	unsigned int mask = 0;
 	struct comedi_file *cfp = file->private_data;
 	struct comedi_device *dev = cfp->dev;
-	struct comedi_subdevice *s;
+	struct comedi_subdevice *s, *s_read;
 
 	mutex_lock(&dev->mutex);
 
@@ -2272,6 +2272,7 @@ static unsigned int comedi_poll(struct file *file, poll_table *wait)
 	}
 
 	s = comedi_file_read_subdevice(file);
+	s_read = s;
 	if (s && s->async) {
 		poll_wait(file, &s->async->wait_head, wait);
 		if (!s->busy || !comedi_is_subdevice_running(s) ||
@@ -2284,7 +2285,8 @@ static unsigned int comedi_poll(struct file *file, poll_table *wait)
 	if (s && s->async) {
 		unsigned int bps = comedi_bytes_per_sample(s);
 
-		poll_wait(file, &s->async->wait_head, wait);
+		if (s != s_read)
+			poll_wait(file, &s->async->wait_head, wait);
 		comedi_buf_write_alloc(s, s->async->prealloc_bufsz);
 		if (!s->busy || !comedi_is_subdevice_running(s) ||
 		    !(s->async->cmd.flags & CMDF_WRITE) ||
-- 
2.6.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1243548 — RE: [PATCH 0/6] staging: comedi: fix some minor issues with file poll op

On Friday, October 09, 2015 4:27 AM, Ian Abbott wrote:
> A few changes for the "poll" file operation to avoid poll-waiting on the
> same subdevice for both read and write (patch 1), avoid allocating write
> buffer space unnecessarily and possibly inappropriately (patch 4),
> consider whether any active commands belong to the current file object
> (patch 5), and avoid using the main mutex (for performance reasons)
> (patch 6).
>
> 1) staging: comedi: don't poll_wait on same subdevice twice
> 2) staging: comedi: rename comedi_buf_write_n_available
> 3) staging: comedi: add new comedi_buf_write_n_available()
> 4) staging: comedi: don't allocate buffer space when polling for write
> 5) staging: comedi: check command started by file being polled
> 6) staging: comedi: don't use mutex when polling file
>
>  drivers/staging/comedi/comedi_buf.c      | 19 +++++++++++++------
>  drivers/staging/comedi/comedi_fops.c     | 17 +++++++++--------
>  drivers/staging/comedi/comedi_internal.h |  1 +
>  3 files changed, 23 insertions(+), 14 deletions(-)

Thanks!

Reviewed-by: H Hartley Sweeten <hsweeten@visionengravers.com>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web