watch_queue: Clean up some code
Subsystems (3)
v3 cover watch_queue: Clean up some code
There is a dangling reference to pipe in a watch_queue after clearing it. Thus, NULL that pointer while clearing.
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 NULLed 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 kerneldoc-styled, so that I can extend the comment mentioning that the pipe is NULL when watch_queue is cleared. In the process, I have also hopefully improved documentation by documenting things which weren't documented before.
Changes in v3:
- Fixed misplaced/incorrect comment for members watch_list and list_node in struct watch.
- Minor rephrase of comment before NULLing in watch_queue_clear().
Changes in v2 (6 Aug 2022):
- Merged the NULLing and removing defunct patches.
- Removed READ_ONCE barrier in lock_wqueue().
- Improved and fixed errors in struct docs.
- Better commit messages.
Siddh Raman Pant (2):
include/linux/watch_queue: Improve documentation kernel/watch_queue: NULL the dangling *pipe, and use it for clear
check
include/linux/watch_queue.h | 100 ++++++++++++++++++++++++++---------- kernel/watch_queue.c | 12 ++--- 2 files changed, 79 insertions(+), 33 deletions(-)
Signature · 1 line
2.39.0
PATCH v3 1/2 include/linux/watch_queue: Improve documentation
Introduce kerneldoc-style comments, and document a couple of things explicitly.
--- include/linux/watch_queue.h | 102 ++++++++++++++++++++++++++---------- 1 file changed, 75 insertions(+), 27 deletions(-)
include/linux/watch_queue.h +75 -27
@@ -18,57 +18,103 @@struct cred ; +/** +* struct 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; }; +/** +* struct watch_filter - Filter on watch +* +* @rcu: RCU head (in union with type_filter) +* @type_filter: Bitmask of accepted types (in union with rcu) +* @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[]; }; +/** +* struct watch_queue - General notification queue +* +* @rcu: RCU head +* @filter: Filter to use on watches +* @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: To serialize accesses and removes +* @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. +/** +* struct watch - Representation of a watch on an object +* +* @rcu: RCU head (in union with info_id) +* @info_id: ID to be OR'd in to info field (in union with rcu) +* @queue: Queue to post events to +* @queue_node: Link in queue->watches +* @watch_list: The watch list containing this watch +* @list_node: Link in watch_list->watchers +* @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. +/** +* struct watch_list - List of watches on an object +* +* @rcu: RCU head +* @watchers: List head +* @release_watch: Function to release watch +* @lock: To protect addition and removal of watches */ struct watch_list {struct rcu_head rcu;
@@ -118,8 +164,10 @@ static inline void remove_watch_list(struct watch_list *wlist, u64 id) }/** -* watch_sizeof - Calculate the information part of the size of a watch record, -* given the structure size. +* watch_sizeof() - Calculate the information part of the size of a watch +* record, given the structure size. +* +* @STRUCT: The structure whose size is to be given */ #define watch_sizeof (STRUCT ) (sizeof(STRUCT) << WATCH_INFO_LENGTH__SHIFT)
Signature · 1 line
2.39.0
David Howells
maintainerRe: [PATCH v3 1/2] include/linux/watch_queue: Improve documentation
Quoted 7 lines · Siddh Raman Pant
include/linux/watch_queue.h +7
-0+/** +* struct 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
include/linux/watch_queue.h line 34
Quoted 10 lines
include/linux/watch_queue.h +4
-3+*/ 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; };
Please don't.
The structure is documented fully here:
Documentation/core-api/watch_queue.rst
See:
https://docs.kernel.org/core-api/watch_queue.html#event-filtering
The three column approach is much more readable in the code as it doesn't separate the descriptions from the things described. Putting things in columns has been around for around 6000 years.
David
Re: [PATCH v3 1/2] include/linux/watch_queue: Improve documentation
Quoted 8 lines · David Howells
Please don't.
The structure is documented fully here:
Documentation/core-api/watch_queue.rstSee:
Quoted 7 lines
https://docs.kernel.org/core-api/watch_queue.html#event-filtering
The three column approach is much more readable in the code as it doesn't separate the descriptions from the things described. Putting things in columns has been around for around 6000 years.
David
Okay. Apologies for that.
But what about the second patch? Should I send that without these doc changes?
Thanks,
Siddh
David Howells
maintainerRe: [PATCH v3 1/2] include/linux/watch_queue: Improve documentation
Quoted 2 lines · Siddh Raman Pant
But what about the second patch? Should I send that without these doc changes?
Can you repost it without the first patch being present?
David
Re: [PATCH v3 1/2] include/linux/watch_queue: Improve documentation
Quoted 1 line · David Howells
Can you repost it without the first patch being present?
Sure.
Thanks,
Siddh
PATCH v3 2/2 kernel/watch_queue: NULL the dangling *pipe, and use it for clear check
In reply to Siddh Raman PantNULL the dangling pipe reference while clearing watch_queue.
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).
The sole use of wqueue->defunct is for checking if the watch queue has been cleared, but wqueue->pipe is also NULLed while clearing.
Thus, wqueue->defunct is superfluous, as wqueue->pipe can be checked for NULL. Hence, the former can be removed.
--- include/linux/watch_queue.h | 4 +--- kernel/watch_queue.c | 12 ++++++------ 2 files changed, 7 insertions(+), 9 deletions(-)
include/linux/watch_queue.h +1 -3
@@ -55,7 +55,7 @@ struct watch_filter {* * @rcu: RCU head * @filter: Filter to use on watches -* @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: To serialize accesses and removes * @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; };/**
kernel/watch_queue.c +6 -6
@@ -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 (!wqueue->pipe)) {spin_unlock_bh (&wqueue->lock);return false; }
@@ -105,9 +105,6 @@ static bool post_one_notification(struct watch_queue *wqueue,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,8 +600,11 @@ 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 can be freed by callers like free_pipe_info(). +* Removing this reference also prevents new notifications. +*/ + wqueue->pipe =NULL ;while (!hlist_empty (&wqueue->watches)) { watch = hlist_entry(wqueue->watches.first, struct watch, queue_node);
Signature · 1 line
2.39.0