linux-kernel-mentees.lists.linuxfoundation.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] kernel/watch_queue: Clean up some code
@ 2022-08-04 13:30 Siddh Raman Pant via Linux-kernel-mentees
  2022-08-04 13:30 ` [PATCH 1/3 v4] kernel/watch_queue: Remove dangling pipe reference while clearing watch_queue Siddh Raman Pant via Linux-kernel-mentees
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Siddh Raman Pant via Linux-kernel-mentees @ 2022-08-04 13:30 UTC (permalink / raw)
  To: Eric Biggers, Jonathan Corbet, David Howells, Randy Dunlap,
	Mauro Carvalho Chehab, Christophe JAILLET, Eric Dumazet
  Cc: linux-kernel-mentees, linux-kernel

There is a dangling reference to pipe in a watch_queue after clearing it.
Thus, NULL that pointer while clearing. This can be thought of as a v4 of
the patches I had sent earlier.

This change renders wqueue->defunct superfluous, as the latter is only used
to check if watch_queue is cleared. With this change, the pipe is NULL'd
while clearing, so we can just check if the pipe is NULL.

Extending comment for watch_queue->pipe in the definition of watch_queue
made the comment conventionally too long (it was already past 80 chars),
so I have changed the struct annotations to be doxygen-styled, so that
I can extend the comment mentioning that the pipe is NULL when watch_queue
is cleared.

Siddh Raman Pant (3):
  kernel/watch_queue: Remove dangling pipe reference while clearing
    watch_queue
  kernel/watch_queue: Improve struct annotation formatting
  kernel/watch_queue: Remove wqueue->defunct and use pipe for clear
    check

 include/linux/watch_queue.h | 95 +++++++++++++++++++++++++++----------
 kernel/watch_queue.c        | 11 ++---
 2 files changed, 75 insertions(+), 31 deletions(-)

-- 
2.35.1


_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/3 v4] kernel/watch_queue: Remove dangling pipe reference while clearing watch_queue
  2022-08-04 13:30 [PATCH 0/3] kernel/watch_queue: Clean up some code Siddh Raman Pant via Linux-kernel-mentees
@ 2022-08-04 13:30 ` Siddh Raman Pant via Linux-kernel-mentees
  2022-08-04 13:30 ` [PATCH 2/3] kernel/watch_queue: Improve struct annotation formatting Siddh Raman Pant via Linux-kernel-mentees
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 13+ messages in thread
From: Siddh Raman Pant via Linux-kernel-mentees @ 2022-08-04 13:30 UTC (permalink / raw)
  To: Eric Biggers, Jonathan Corbet, David Howells, Randy Dunlap,
	Mauro Carvalho Chehab, Christophe JAILLET, Eric Dumazet
  Cc: linux-kernel-mentees, linux-kernel

If not done, a reference to a freed pipe remains in the watch_queue,
as this function is called before freeing a pipe in free_pipe_info()
(see line 834 of fs/pipe.c).

We also need to use READ_ONCE() in post_one_notification() to prevent the
compiler from optimising and loading a non-NULL value from wqueue->pipe.

Signed-off-by: Siddh Raman Pant <code@siddh.me>
---
Changes in v4:
- Brought the lines towards the start rather than the end.
- Removed incorrect NULLing of wqueue->pipe->watch_queue.
The latter was pointed out by Eric Biggers <ebiggers@kernel.org>
in reply to v3.

Changes in v3:
- Restored the original unlock order, and clear before unlock.
- Used READ_ONCE() in post path.
This was explained by David Howells <dhowells@redhat.com> in
reply to v1.

Changes in v2:
- Removed the superfluous ifdef guard.

 kernel/watch_queue.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/kernel/watch_queue.c b/kernel/watch_queue.c
index a6f9bdd956c3..8999c4e3076d 100644
--- a/kernel/watch_queue.c
+++ b/kernel/watch_queue.c
@@ -99,7 +99,7 @@ static bool post_one_notification(struct watch_queue *wqueue,
 				  struct watch_notification *n)
 {
 	void *p;
-	struct pipe_inode_info *pipe = wqueue->pipe;
+	struct pipe_inode_info *pipe = READ_ONCE(wqueue->pipe);
 	struct pipe_buffer *buf;
 	struct page *page;
 	unsigned int head, tail, mask, note, offset, len;
@@ -606,6 +606,9 @@ void watch_queue_clear(struct watch_queue *wqueue)
 	/* Prevent new notifications from being stored. */
 	wqueue->defunct = true;
 
+	/* This pipe will get freed by caller, and we are anyways clearing. */
+	wqueue->pipe = NULL;
+
 	while (!hlist_empty(&wqueue->watches)) {
 		watch = hlist_entry(wqueue->watches.first, struct watch, queue_node);
 		hlist_del_init_rcu(&watch->queue_node);
-- 
2.35.1


_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/3] kernel/watch_queue: Improve struct annotation formatting
  2022-08-04 13:30 [PATCH 0/3] kernel/watch_queue: Clean up some code Siddh Raman Pant via Linux-kernel-mentees
  2022-08-04 13:30 ` [PATCH 1/3 v4] kernel/watch_queue: Remove dangling pipe reference while clearing watch_queue Siddh Raman Pant via Linux-kernel-mentees
@ 2022-08-04 13:30 ` Siddh Raman Pant via Linux-kernel-mentees
  2022-08-05  7:22   ` Eric Biggers
  2022-08-04 13:30 ` [PATCH 3/3] kernel/watch_queue: Remove wqueue->defunct and use pipe for clear check Siddh Raman Pant via Linux-kernel-mentees
  2022-08-05  7:16 ` [PATCH 0/3] kernel/watch_queue: Clean up some code Eric Biggers
  3 siblings, 1 reply; 13+ messages in thread
From: Siddh Raman Pant via Linux-kernel-mentees @ 2022-08-04 13:30 UTC (permalink / raw)
  To: Eric Biggers, Jonathan Corbet, David Howells, Randy Dunlap,
	Mauro Carvalho Chehab, Christophe JAILLET, Eric Dumazet
  Cc: linux-kernel-mentees, linux-kernel

Improve formatting struct annotations in watch_queue.h, so that they
fall in the preferred 80 character limit.

Signed-off-by: Siddh Raman Pant <code@siddh.me>
---
 include/linux/watch_queue.h | 96 +++++++++++++++++++++++++++----------
 1 file changed, 71 insertions(+), 25 deletions(-)

diff --git a/include/linux/watch_queue.h b/include/linux/watch_queue.h
index fc6bba20273b..c99c39ec6548 100644
--- a/include/linux/watch_queue.h
+++ b/include/linux/watch_queue.h
@@ -18,57 +18,103 @@
 
 struct cred;
 
+/**
+ * watch_type_filter - Filter on watch type
+ *
+ * @type: Type of watch_notification
+ * @subtype_filter: Bitmask of subtypes to filter on
+ * @info_filter: Filter on watch_notification::info
+ * @info_mask: Mask of relevant bits in info_filter
+ */
 struct watch_type_filter {
 	enum watch_notification_type type;
-	__u32		subtype_filter[1];	/* Bitmask of subtypes to filter on */
-	__u32		info_filter;		/* Filter on watch_notification::info */
-	__u32		info_mask;		/* Mask of relevant bits in info_filter */
+	__u32		subtype_filter[1];
+	__u32		info_filter;
+	__u32		info_mask;
 };
 
+/**
+ * watch_filter - Filter on watch
+ *
+ * @rcu: (union) RCU head
+ * @type_filter: (union) Bitmask of accepted types
+ * @nr_filters: Number of filters
+ * @filters: Array of watch_type_filter
+ */
 struct watch_filter {
 	union {
 		struct rcu_head	rcu;
-		/* Bitmask of accepted types */
 		DECLARE_BITMAP(type_filter, WATCH_TYPE__NR);
 	};
-	u32			nr_filters;	/* Number of filters */
+	u32			 nr_filters;
 	struct watch_type_filter filters[];
 };
 
+/**
+ * watch_queue - General notification queue
+ *
+ * @rcu: RCU head
+ * @filter: Filter on watch_notification::info
+ * @pipe: The pipe we're using as a buffer.
+ * @watches: Contributory watches
+ * @notes: Preallocated notifications
+ * @notes_bitmap: Allocation bitmap for notes
+ * @usage: Object usage count
+ * @lock: Spinlock
+ * @nr_notes: Number of notes
+ * @nr_pages: Number of pages in notes[]
+ * @defunct: True when queues closed
+ */
 struct watch_queue {
 	struct rcu_head		rcu;
 	struct watch_filter __rcu *filter;
-	struct pipe_inode_info	*pipe;		/* The pipe we're using as a buffer */
-	struct hlist_head	watches;	/* Contributory watches */
-	struct page		**notes;	/* Preallocated notifications */
-	unsigned long		*notes_bitmap;	/* Allocation bitmap for notes */
-	struct kref		usage;		/* Object usage count */
+	struct pipe_inode_info	*pipe;
+	struct hlist_head	watches;
+	struct page		**notes;
+	unsigned long		*notes_bitmap;
+	struct kref		usage;
 	spinlock_t		lock;
-	unsigned int		nr_notes;	/* Number of notes */
-	unsigned int		nr_pages;	/* Number of pages in notes[] */
-	bool			defunct;	/* T when queues closed */
+	unsigned int		nr_notes;
+	unsigned int		nr_pages;
+	bool			defunct;
 };
 
-/*
- * Representation of a watch on an object.
+/**
+ * watch - Representation of a watch on an object.
+ *
+ * @rcu: (union) RCU head
+ * @info_id: (union) ID to be OR'd in to info field
+ * @queue: Queue to post events to
+ * @queue_node: Link in queue->watches
+ * @watch_list: Link in watch_list->watchers
+ * @list_node: The list node
+ * @cred: Creds of the owner of the watch
+ * @private: Private data for the watched object
+ * @id: Internal identifier
+ * @usage: Object usage count
  */
 struct watch {
 	union {
 		struct rcu_head	rcu;
-		u32		info_id;	/* ID to be OR'd in to info field */
+		u32		info_id;
 	};
-	struct watch_queue __rcu *queue;	/* Queue to post events to */
-	struct hlist_node	queue_node;	/* Link in queue->watches */
+	struct watch_queue __rcu *queue;
+	struct hlist_node	queue_node;
 	struct watch_list __rcu	*watch_list;
-	struct hlist_node	list_node;	/* Link in watch_list->watchers */
-	const struct cred	*cred;		/* Creds of the owner of the watch */
-	void			*private;	/* Private data for the watched object */
-	u64			id;		/* Internal identifier */
-	struct kref		usage;		/* Object usage count */
+	struct hlist_node	list_node;
+	const struct cred	*cred;
+	void			*private;
+	u64			id;
+	struct kref		usage;
 };
 
-/*
- * List of watches on an object.
+/**
+ * watch_list - List of watches on an object.
+ *
+ * @rcu: RCU head
+ * @watchers: List head
+ * @release_watch: Function to release watch
+ * @lock: Spinlock
  */
 struct watch_list {
 	struct rcu_head		rcu;
-- 
2.35.1


_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 3/3] kernel/watch_queue: Remove wqueue->defunct and use pipe for clear check
  2022-08-04 13:30 [PATCH 0/3] kernel/watch_queue: Clean up some code Siddh Raman Pant via Linux-kernel-mentees
  2022-08-04 13:30 ` [PATCH 1/3 v4] kernel/watch_queue: Remove dangling pipe reference while clearing watch_queue Siddh Raman Pant via Linux-kernel-mentees
  2022-08-04 13:30 ` [PATCH 2/3] kernel/watch_queue: Improve struct annotation formatting Siddh Raman Pant via Linux-kernel-mentees
@ 2022-08-04 13:30 ` Siddh Raman Pant via Linux-kernel-mentees
  2022-08-04 14:41   ` [PATCH v2 " Siddh Raman Pant via Linux-kernel-mentees
  2022-08-05  7:16 ` [PATCH 0/3] kernel/watch_queue: Clean up some code Eric Biggers
  3 siblings, 1 reply; 13+ messages in thread
From: Siddh Raman Pant via Linux-kernel-mentees @ 2022-08-04 13:30 UTC (permalink / raw)
  To: Eric Biggers, Jonathan Corbet, David Howells, Randy Dunlap,
	Mauro Carvalho Chehab, Christophe JAILLET, Eric Dumazet
  Cc: linux-kernel-mentees, linux-kernel

The sole use of wqueue->defunct is for checking if the watch queue has
been cleared, but wqueue->pipe is also NULL'd while clearing.

Thus, wqueue->defunct is superfluous, as wqueue->pipe can be checked
for NULL.

Signed-off-by: Siddh Raman Pant <code@siddh.me>
---
 include/linux/watch_queue.h |  3 +--
 kernel/watch_queue.c        | 14 +++++---------
 2 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/include/linux/watch_queue.h b/include/linux/watch_queue.h
index c99c39ec6548..2a3b318db49d 100644
--- a/include/linux/watch_queue.h
+++ b/include/linux/watch_queue.h
@@ -55,7 +55,7 @@ struct watch_filter {
  *
  * @rcu: RCU head
  * @filter: Filter on watch_notification::info
- * @pipe: The pipe we're using as a buffer.
+ * @pipe: The pipe we're using as a buffer, NULL when queue is cleared/closed
  * @watches: Contributory watches
  * @notes: Preallocated notifications
  * @notes_bitmap: Allocation bitmap for notes
@@ -63,7 +63,6 @@ struct watch_filter {
  * @lock: Spinlock
  * @nr_notes: Number of notes
  * @nr_pages: Number of pages in notes[]
- * @defunct: True when queues closed
  */
 struct watch_queue {
 	struct rcu_head		rcu;
diff --git a/kernel/watch_queue.c b/kernel/watch_queue.c
index 8999c4e3076d..c63b128818f4 100644
--- a/kernel/watch_queue.c
+++ b/kernel/watch_queue.c
@@ -43,7 +43,7 @@ MODULE_LICENSE("GPL");
 static inline bool lock_wqueue(struct watch_queue *wqueue)
 {
 	spin_lock_bh(&wqueue->lock);
-	if (unlikely(wqueue->defunct)) {
+	if (unlikely(READ_ONCE(wqueue->pipe) == NULL)) {
 		spin_unlock_bh(&wqueue->lock);
 		return false;
 	}
@@ -99,15 +99,12 @@ static bool post_one_notification(struct watch_queue *wqueue,
 				  struct watch_notification *n)
 {
 	void *p;
-	struct pipe_inode_info *pipe = READ_ONCE(wqueue->pipe);
+	struct pipe_inode_info *pipe = wqueue->pipe;
 	struct pipe_buffer *buf;
 	struct page *page;
 	unsigned int head, tail, mask, note, offset, len;
 	bool done = false;
 
-	if (!pipe)
-		return false;
-
 	spin_lock_irq(&pipe->rd_wait.lock);
 
 	mask = pipe->ring_size - 1;
@@ -603,10 +600,9 @@ void watch_queue_clear(struct watch_queue *wqueue)
 	rcu_read_lock();
 	spin_lock_bh(&wqueue->lock);
 
-	/* Prevent new notifications from being stored. */
-	wqueue->defunct = true;
-
-	/* This pipe will get freed by caller, and we are anyways clearing. */
+	/* This pipe will get freed by the caller free_pipe_info().
+	 * Removing this reference also prevents new notifications.
+	 */
 	wqueue->pipe = NULL;
 
 	while (!hlist_empty(&wqueue->watches)) {
-- 
2.35.1


_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 3/3] kernel/watch_queue: Remove wqueue->defunct and use pipe for clear check
  2022-08-04 13:30 ` [PATCH 3/3] kernel/watch_queue: Remove wqueue->defunct and use pipe for clear check Siddh Raman Pant via Linux-kernel-mentees
@ 2022-08-04 14:41   ` Siddh Raman Pant via Linux-kernel-mentees
  2022-08-05  7:24     ` Eric Biggers
  0 siblings, 1 reply; 13+ messages in thread
From: Siddh Raman Pant via Linux-kernel-mentees @ 2022-08-04 14:41 UTC (permalink / raw)
  To: code
  Cc: ebiggers, corbet, rdunlap, linux-kernel, dhowells, edumazet,
	christophe.jaillet, mchehab, linux-kernel-mentees

The sole use of wqueue->defunct is for checking if the watch queue has
been cleared, but wqueue->pipe is also NULL'd while clearing.

Thus, wqueue->defunct is superfluous, as wqueue->pipe can be checked
for NULL.

Signed-off-by: Siddh Raman Pant <code@siddh.me>
---
Changes in v2:
- The sent patch had accidentally missed removing the member
  from struct, fix that.
- Use !READ_ONCE() instead of == NULL, as said by checkpatch.pl.

 include/linux/watch_queue.h |  4 +---
 kernel/watch_queue.c        | 14 +++++---------
 2 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/include/linux/watch_queue.h b/include/linux/watch_queue.h
index c99c39ec6548..88360771c097 100644
--- a/include/linux/watch_queue.h
+++ b/include/linux/watch_queue.h
@@ -55,7 +55,7 @@ struct watch_filter {
  *
  * @rcu: RCU head
  * @filter: Filter on watch_notification::info
- * @pipe: The pipe we're using as a buffer.
+ * @pipe: The pipe we're using as a buffer, NULL when queue is cleared/closed
  * @watches: Contributory watches
  * @notes: Preallocated notifications
  * @notes_bitmap: Allocation bitmap for notes
@@ -63,7 +63,6 @@ struct watch_filter {
  * @lock: Spinlock
  * @nr_notes: Number of notes
  * @nr_pages: Number of pages in notes[]
- * @defunct: True when queues closed
  */
 struct watch_queue {
 	struct rcu_head		rcu;
@@ -76,7 +75,6 @@ struct watch_queue {
 	spinlock_t		lock;
 	unsigned int		nr_notes;
 	unsigned int		nr_pages;
-	bool			defunct;
 };
 
 /**
diff --git a/kernel/watch_queue.c b/kernel/watch_queue.c
index 8999c4e3076d..825943cf74b2 100644
--- a/kernel/watch_queue.c
+++ b/kernel/watch_queue.c
@@ -43,7 +43,7 @@ MODULE_LICENSE("GPL");
 static inline bool lock_wqueue(struct watch_queue *wqueue)
 {
 	spin_lock_bh(&wqueue->lock);
-	if (unlikely(wqueue->defunct)) {
+	if (unlikely(!READ_ONCE(wqueue->pipe))) {
 		spin_unlock_bh(&wqueue->lock);
 		return false;
 	}
@@ -99,15 +99,12 @@ static bool post_one_notification(struct watch_queue *wqueue,
 				  struct watch_notification *n)
 {
 	void *p;
-	struct pipe_inode_info *pipe = READ_ONCE(wqueue->pipe);
+	struct pipe_inode_info *pipe = wqueue->pipe;
 	struct pipe_buffer *buf;
 	struct page *page;
 	unsigned int head, tail, mask, note, offset, len;
 	bool done = false;
 
-	if (!pipe)
-		return false;
-
 	spin_lock_irq(&pipe->rd_wait.lock);
 
 	mask = pipe->ring_size - 1;
@@ -603,10 +600,9 @@ void watch_queue_clear(struct watch_queue *wqueue)
 	rcu_read_lock();
 	spin_lock_bh(&wqueue->lock);
 
-	/* Prevent new notifications from being stored. */
-	wqueue->defunct = true;
-
-	/* This pipe will get freed by caller, and we are anyways clearing. */
+	/* This pipe will get freed by the caller free_pipe_info().
+	 * Removing this reference also prevents new notifications.
+	 */
 	wqueue->pipe = NULL;
 
 	while (!hlist_empty(&wqueue->watches)) {
-- 
2.35.1


_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/3] kernel/watch_queue: Clean up some code
  2022-08-04 13:30 [PATCH 0/3] kernel/watch_queue: Clean up some code Siddh Raman Pant via Linux-kernel-mentees
                   ` (2 preceding siblings ...)
  2022-08-04 13:30 ` [PATCH 3/3] kernel/watch_queue: Remove wqueue->defunct and use pipe for clear check Siddh Raman Pant via Linux-kernel-mentees
@ 2022-08-05  7:16 ` Eric Biggers
  2022-08-05  9:35   ` Siddh Raman Pant via Linux-kernel-mentees
  3 siblings, 1 reply; 13+ messages in thread
From: Eric Biggers @ 2022-08-05  7:16 UTC (permalink / raw)
  To: Siddh Raman Pant
  Cc: Jonathan Corbet, Randy Dunlap, linux-kernel, David Howells,
	Eric Dumazet, Christophe JAILLET, Mauro Carvalho Chehab,
	linux-kernel-mentees

On Thu, Aug 04, 2022 at 07:00:21PM +0530, Siddh Raman Pant wrote:
> There is a dangling reference to pipe in a watch_queue after clearing it.
> Thus, NULL that pointer while clearing. This can be thought of as a v4 of
> the patches I had sent earlier.
> 
> This change renders wqueue->defunct superfluous, as the latter is only used
> to check if watch_queue is cleared. With this change, the pipe is NULL'd
> while clearing, so we can just check if the pipe is NULL.
> 
> Extending comment for watch_queue->pipe in the definition of watch_queue
> made the comment conventionally too long (it was already past 80 chars),
> so I have changed the struct annotations to be doxygen-styled, so that
> I can extend the comment mentioning that the pipe is NULL when watch_queue
> is cleared.
> 
> Siddh Raman Pant (3):
>   kernel/watch_queue: Remove dangling pipe reference while clearing
>     watch_queue
>   kernel/watch_queue: Improve struct annotation formatting
>   kernel/watch_queue: Remove wqueue->defunct and use pipe for clear
>     check
> 
>  include/linux/watch_queue.h | 95 +++++++++++++++++++++++++++----------
>  kernel/watch_queue.c        | 11 ++---
>  2 files changed, 75 insertions(+), 31 deletions(-)

I think patches 1 and 3 should be merged together.

Also, please use a consistent version number for all patches in the series.  You
have a version 1, version 2, and version 4 patch all in the same series, which
is very confusing.

- Eric
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/3] kernel/watch_queue: Improve struct annotation formatting
  2022-08-04 13:30 ` [PATCH 2/3] kernel/watch_queue: Improve struct annotation formatting Siddh Raman Pant via Linux-kernel-mentees
@ 2022-08-05  7:22   ` Eric Biggers
  2022-08-05  9:35     ` Siddh Raman Pant via Linux-kernel-mentees
  0 siblings, 1 reply; 13+ messages in thread
From: Eric Biggers @ 2022-08-05  7:22 UTC (permalink / raw)
  To: Siddh Raman Pant
  Cc: Jonathan Corbet, Randy Dunlap, linux-kernel, David Howells,
	Eric Dumazet, Christophe JAILLET, Mauro Carvalho Chehab,
	linux-kernel-mentees

On Thu, Aug 04, 2022 at 07:00:23PM +0530, Siddh Raman Pant wrote:
> Improve formatting struct annotations in watch_queue.h, so that they
> fall in the preferred 80 character limit.
> 
> Signed-off-by: Siddh Raman Pant <code@siddh.me>

This patch isn't just fixing overly long lines, but rather is introducing
kerneldoc comments and documenting things that weren't documented before.
That's fine, but please make the commit message accurately describe the patch.

> diff --git a/include/linux/watch_queue.h b/include/linux/watch_queue.h
> index fc6bba20273b..c99c39ec6548 100644
> --- a/include/linux/watch_queue.h
> +++ b/include/linux/watch_queue.h
> @@ -18,57 +18,103 @@
>  
>  struct cred;
>  
> +/**
> + * watch_type_filter - Filter on watch type

If you're going to use kerneldoc comments, they should be correctly formatted.
This is not, since it's missing the word struct.  You can run this command to
see the kerneldoc warnings:

	./scripts/kernel-doc -v -none include/linux/watch_queue.h

> + * @lock: Spinlock

Please make sure that comments provide useful information and don't just repeat
what the code says.

- Eric
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 3/3] kernel/watch_queue: Remove wqueue->defunct and use pipe for clear check
  2022-08-04 14:41   ` [PATCH v2 " Siddh Raman Pant via Linux-kernel-mentees
@ 2022-08-05  7:24     ` Eric Biggers
  2022-08-05  9:35       ` Siddh Raman Pant via Linux-kernel-mentees
  0 siblings, 1 reply; 13+ messages in thread
From: Eric Biggers @ 2022-08-05  7:24 UTC (permalink / raw)
  To: Siddh Raman Pant
  Cc: corbet, rdunlap, linux-kernel, dhowells, edumazet,
	christophe.jaillet, mchehab, linux-kernel-mentees

On Thu, Aug 04, 2022 at 08:11:52PM +0530, Siddh Raman Pant wrote:
>  static inline bool lock_wqueue(struct watch_queue *wqueue)
>  {
>  	spin_lock_bh(&wqueue->lock);
> -	if (unlikely(wqueue->defunct)) {
> +	if (unlikely(!READ_ONCE(wqueue->pipe))) {
>  		spin_unlock_bh(&wqueue->lock);
>  		return false;
>  	}

Why is the READ_ONCE() needed?  Doesn't wqueue->lock protect wqueue->pipe?

> +	/* This pipe will get freed by the caller free_pipe_info().
> +	 * Removing this reference also prevents new notifications.
> +	 */

This isn't the correct block comment format; it should look like:

	/*
	 * This pipe will get freed by the caller free_pipe_info().
	 * Removing this reference also prevents new notifications.
	 */

- Eric
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/3] kernel/watch_queue: Clean up some code
  2022-08-05  7:16 ` [PATCH 0/3] kernel/watch_queue: Clean up some code Eric Biggers
@ 2022-08-05  9:35   ` Siddh Raman Pant via Linux-kernel-mentees
  0 siblings, 0 replies; 13+ messages in thread
From: Siddh Raman Pant via Linux-kernel-mentees @ 2022-08-05  9:35 UTC (permalink / raw)
  To: Eric Biggers
  Cc: Jonathan Corbet, Randy Dunlap, linux-kernel, David Howells,
	Eric Dumazet, Christophe JAILLET, Mauro Carvalho Chehab,
	linux-kernel-mentees

On Fri, 05 Aug 2022 12:46:17 +0530  Eric Biggers  wrote:
> I think patches 1 and 3 should be merged together.
> 
> Also, please use a consistent version number for all patches in the series.  You
> have a version 1, version 2, and version 4 patch all in the same series, which
> is very confusing.
> 
> - Eric
> 

Will do.

Sorry for the confusion.

Thanks,
Siddh
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/3] kernel/watch_queue: Improve struct annotation formatting
  2022-08-05  7:22   ` Eric Biggers
@ 2022-08-05  9:35     ` Siddh Raman Pant via Linux-kernel-mentees
  0 siblings, 0 replies; 13+ messages in thread
From: Siddh Raman Pant via Linux-kernel-mentees @ 2022-08-05  9:35 UTC (permalink / raw)
  To: Eric Biggers
  Cc: Jonathan Corbet, Randy Dunlap, linux-kernel, David Howells,
	Eric Dumazet, Christophe JAILLET, Mauro Carvalho Chehab,
	linux-kernel-mentees

On Fri, 05 Aug 2022 12:52:11 +0530  Eric Biggers  wrote:
> On Thu, Aug 04, 2022 at 07:00:23PM +0530, Siddh Raman Pant wrote:
> > Improve formatting struct annotations in watch_queue.h, so that they
> > fall in the preferred 80 character limit.
> > 
> > Signed-off-by: Siddh Raman Pant code@siddh.me>
> 
> This patch isn't just fixing overly long lines, but rather is introducing
> kerneldoc comments and documenting things that weren't documented before.
> That's fine, but please make the commit message accurately describe the patch.
> 
> > diff --git a/include/linux/watch_queue.h b/include/linux/watch_queue.h
> > index fc6bba20273b..c99c39ec6548 100644
> > --- a/include/linux/watch_queue.h
> > +++ b/include/linux/watch_queue.h
> > @@ -18,57 +18,103 @@
> >  
> >  struct cred;
> >  
> > +/**
> > + * watch_type_filter - Filter on watch type
> 
> If you're going to use kerneldoc comments, they should be correctly formatted.
> This is not, since it's missing the word struct.  You can run this command to
> see the kerneldoc warnings:
> 
> 	./scripts/kernel-doc -v -none include/linux/watch_queue.h
> 
> > + * @lock: Spinlock
> 
> Please make sure that comments provide useful information and don't just repeat
> what the code says.
> 
> - Eric
> 

Okay, will do.

Thanks,
Siddh
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 3/3] kernel/watch_queue: Remove wqueue->defunct and use pipe for clear check
  2022-08-05  7:24     ` Eric Biggers
@ 2022-08-05  9:35       ` Siddh Raman Pant via Linux-kernel-mentees
  2022-08-05 18:33         ` Eric Biggers
  0 siblings, 1 reply; 13+ messages in thread
From: Siddh Raman Pant via Linux-kernel-mentees @ 2022-08-05  9:35 UTC (permalink / raw)
  To: Eric Biggers
  Cc: corbet, rdunlap, linux-kernel, dhowells, edumazet,
	christophe.jaillet, mchehab, linux-kernel-mentees

On Fri, 05 Aug 2022 12:54:31 +0530  Eric Biggers  wrote:
> Why is the READ_ONCE() needed?  Doesn't wqueue->lock protect wqueue->pipe?

We are changing the pointer while a notification can be potentially waiting to
be posted to the pipe. So a barrier is needed to prevent compiler magic from
reloading the value.

This was remarked by David Howells here:
https://lore.kernel.org/lkml/3558070.1658933200@warthog.procyon.org.uk/

> This isn't the correct block comment format; it should look like:
> 
>      /*
>       * This pipe will get freed by the caller free_pipe_info().
>       * Removing this reference also prevents new notifications.
>       */
> 
> - Eric
> 

Okay, will make the change.

Thanks,
Siddh
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 3/3] kernel/watch_queue: Remove wqueue->defunct and use pipe for clear check
  2022-08-05  9:35       ` Siddh Raman Pant via Linux-kernel-mentees
@ 2022-08-05 18:33         ` Eric Biggers
  2022-08-06  7:23           ` Siddh Raman Pant via Linux-kernel-mentees
  0 siblings, 1 reply; 13+ messages in thread
From: Eric Biggers @ 2022-08-05 18:33 UTC (permalink / raw)
  To: Siddh Raman Pant
  Cc: corbet, rdunlap, linux-kernel, dhowells, edumazet,
	christophe.jaillet, mchehab, linux-kernel-mentees

On Fri, Aug 05, 2022 at 03:05:41PM +0530, Siddh Raman Pant wrote:
> On Fri, 05 Aug 2022 12:54:31 +0530  Eric Biggers  wrote:
> > Why is the READ_ONCE() needed?  Doesn't wqueue->lock protect wqueue->pipe?
> 
> We are changing the pointer while a notification can be potentially waiting to
> be posted to the pipe. So a barrier is needed to prevent compiler magic from
> reloading the value.
> 

wqueue->pipe is only read or written while wqueue->lock is held, so that is not
an issue at all.

- Eric
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 3/3] kernel/watch_queue: Remove wqueue->defunct and use pipe for clear check
  2022-08-05 18:33         ` Eric Biggers
@ 2022-08-06  7:23           ` Siddh Raman Pant via Linux-kernel-mentees
  0 siblings, 0 replies; 13+ messages in thread
From: Siddh Raman Pant via Linux-kernel-mentees @ 2022-08-06  7:23 UTC (permalink / raw)
  To: Eric Biggers
  Cc: corbet, rdunlap, linux-kernel, dhowells, edumazet,
	christophe.jaillet, mchehab, linux-kernel-mentees

On Sat, 06 Aug 2022 00:03:20 +0530  Eric Biggers  wrote:
> wqueue->pipe is only read or written while wqueue->lock is held, so that is not
> an issue at all.
> 
> - Eric

Thanks for explaining. I will send the v2 now.

Thanks,
Siddh
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2022-08-06  7:23 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-04 13:30 [PATCH 0/3] kernel/watch_queue: Clean up some code Siddh Raman Pant via Linux-kernel-mentees
2022-08-04 13:30 ` [PATCH 1/3 v4] kernel/watch_queue: Remove dangling pipe reference while clearing watch_queue Siddh Raman Pant via Linux-kernel-mentees
2022-08-04 13:30 ` [PATCH 2/3] kernel/watch_queue: Improve struct annotation formatting Siddh Raman Pant via Linux-kernel-mentees
2022-08-05  7:22   ` Eric Biggers
2022-08-05  9:35     ` Siddh Raman Pant via Linux-kernel-mentees
2022-08-04 13:30 ` [PATCH 3/3] kernel/watch_queue: Remove wqueue->defunct and use pipe for clear check Siddh Raman Pant via Linux-kernel-mentees
2022-08-04 14:41   ` [PATCH v2 " Siddh Raman Pant via Linux-kernel-mentees
2022-08-05  7:24     ` Eric Biggers
2022-08-05  9:35       ` Siddh Raman Pant via Linux-kernel-mentees
2022-08-05 18:33         ` Eric Biggers
2022-08-06  7:23           ` Siddh Raman Pant via Linux-kernel-mentees
2022-08-05  7:16 ` [PATCH 0/3] kernel/watch_queue: Clean up some code Eric Biggers
2022-08-05  9:35   ` Siddh Raman Pant via Linux-kernel-mentees

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).