| Message ID | 20250403-fuse-io-uring-trace-points-v3-1-35340aa31d9c@ddn.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fuse: Improve ftraces, per-cpu req unique and code dup removal | expand |
On Thu, 3 Apr 2025 at 22:23, Bernd Schubert <bschubert@ddn.com> wrote: > +/** > + * Get the next unique ID for a request > + */ > +static inline u64 fuse_get_unique(struct fuse_iqueue *fiq) > +{ > + int step = FUSE_REQ_ID_STEP * (task_cpu(current)); > + u64 cntr = this_cpu_inc_return(*fiq->reqctr); > + > + return cntr * FUSE_REQ_ID_STEP * NR_CPUS + step; Thinking a bit... this looks wrong. The reason is that the task could be migrated to a different CPU between the task_cpu() and the this_cpu_inc_return(), resulting in a possibly duplicated value. This could be fixed with a preempt_disable()/preempt_enable() pair, but I think it would be cleaner to go with my original idea and initialize the percpu counters to CPUID and increment by NR_CPU * FUSE_REQ_ID_STEP when fetching a new value. Thanks, Miklos
On 4/4/25 14:43, Miklos Szeredi wrote: > On Thu, 3 Apr 2025 at 22:23, Bernd Schubert <bschubert@ddn.com> wrote: > >> +/** >> + * Get the next unique ID for a request >> + */ >> +static inline u64 fuse_get_unique(struct fuse_iqueue *fiq) >> +{ >> + int step = FUSE_REQ_ID_STEP * (task_cpu(current)); >> + u64 cntr = this_cpu_inc_return(*fiq->reqctr); >> + >> + return cntr * FUSE_REQ_ID_STEP * NR_CPUS + step; > > Thinking a bit... this looks wrong. > > The reason is that the task could be migrated to a different CPU > between the task_cpu() and the this_cpu_inc_return(), resulting in a > possibly duplicated value. > > This could be fixed with a preempt_disable()/preempt_enable() pair, > but I think it would be cleaner to go with my original idea and > initialize the percpu counters to CPUID and increment by NR_CPU * > FUSE_REQ_ID_STEP when fetching a new value. > Oh right, I guess something like this diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h index 80a526eaba38..eac26ee654ca 100644 --- a/fs/fuse/fuse_i.h +++ b/fs/fuse/fuse_i.h @@ -1074,10 +1074,7 @@ static inline void fuse_sync_bucket_dec(struct fuse_sync_bucket *bucket) */ static inline u64 fuse_get_unique(struct fuse_iqueue *fiq) { - int step = FUSE_REQ_ID_STEP * (task_cpu(current)); - u64 cntr = this_cpu_inc_return(*fiq->reqctr); - - return cntr * FUSE_REQ_ID_STEP * NR_CPUS + step; + return this_cpu_add_return(*fiq->reqctr, FUSE_REQ_ID_STEP * NR_CPUS); } /** Device operations */ diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c index 7e1066c174d0..463cf7797e1b 100644 --- a/fs/fuse/inode.c +++ b/fs/fuse/inode.c @@ -930,7 +930,13 @@ static void fuse_iqueue_init(struct fuse_iqueue *fiq, memset(fiq, 0, sizeof(struct fuse_iqueue)); spin_lock_init(&fiq->lock); init_waitqueue_head(&fiq->waitq); + int cpu; + fiq->reqctr = alloc_percpu(u64); + for_each_possible_cpu(cpu) { + *per_cpu_ptr(fiq->reqctr, cpu) = cpu * FUSE_REQ_ID_STEP; + } + INIT_LIST_HEAD(&fiq->pending); INIT_LIST_HEAD(&fiq->interrupts); fiq->forget_list_tail = &fiq->forget_list_head; First need to test and think about it again and currently busy with something else - new version follows later. Thanks, Bernd
diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c index 51e31df4c54613280a9c295f530b18e1d461a974..e9592ab092b948bacb5034018bd1f32c917d5c9f 100644 --- a/fs/fuse/dev.c +++ b/fs/fuse/dev.c @@ -204,24 +204,6 @@ unsigned int fuse_len_args(unsigned int numargs, struct fuse_arg *args) } EXPORT_SYMBOL_GPL(fuse_len_args); -static u64 fuse_get_unique_locked(struct fuse_iqueue *fiq) -{ - fiq->reqctr += FUSE_REQ_ID_STEP; - return fiq->reqctr; -} - -u64 fuse_get_unique(struct fuse_iqueue *fiq) -{ - u64 ret; - - spin_lock(&fiq->lock); - ret = fuse_get_unique_locked(fiq); - spin_unlock(&fiq->lock); - - return ret; -} -EXPORT_SYMBOL_GPL(fuse_get_unique); - unsigned int fuse_req_hash(u64 unique) { return hash_long(unique & ~FUSE_INT_REQ_BIT, FUSE_PQ_HASH_BITS); @@ -278,7 +260,7 @@ static void fuse_dev_queue_req(struct fuse_iqueue *fiq, struct fuse_req *req) spin_lock(&fiq->lock); if (fiq->connected) { if (req->in.h.opcode != FUSE_NOTIFY_REPLY) - req->in.h.unique = fuse_get_unique_locked(fiq); + req->in.h.unique = fuse_get_unique(fiq); list_add_tail(&req->list, &fiq->pending); fuse_dev_wake_and_unlock(fiq); } else { @@ -1177,7 +1159,7 @@ __releases(fiq->lock) struct fuse_in_header ih = { .opcode = FUSE_FORGET, .nodeid = forget->forget_one.nodeid, - .unique = fuse_get_unique_locked(fiq), + .unique = fuse_get_unique(fiq), .len = sizeof(ih) + sizeof(arg), }; @@ -1208,7 +1190,7 @@ __releases(fiq->lock) struct fuse_batch_forget_in arg = { .count = 0 }; struct fuse_in_header ih = { .opcode = FUSE_BATCH_FORGET, - .unique = fuse_get_unique_locked(fiq), + .unique = fuse_get_unique(fiq), .len = sizeof(ih) + sizeof(arg), }; diff --git a/fs/fuse/fuse_dev_i.h b/fs/fuse/fuse_dev_i.h index 3b2bfe1248d3573abe3b144a6d4bf6a502f56a40..e0afd837a8024450bab77312c7eebdcc7a39bd36 100644 --- a/fs/fuse/fuse_dev_i.h +++ b/fs/fuse/fuse_dev_i.h @@ -8,10 +8,6 @@ #include <linux/types.h> -/* Ordinary requests have even IDs, while interrupts IDs are odd */ -#define FUSE_INT_REQ_BIT (1ULL << 0) -#define FUSE_REQ_ID_STEP (1ULL << 1) - struct fuse_arg; struct fuse_args; struct fuse_pqueue; diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h index fee96fe7887b30cd57b8a6bbda11447a228cf446..80a526eaba38aa97f6a6faa60e5276fcd7f2668f 100644 --- a/fs/fuse/fuse_i.h +++ b/fs/fuse/fuse_i.h @@ -44,6 +44,10 @@ /** Number of dentries for each connection in the control filesystem */ #define FUSE_CTL_NUM_DENTRIES 5 +/* Ordinary requests have even IDs, while interrupts IDs are odd */ +#define FUSE_INT_REQ_BIT (1ULL << 0) +#define FUSE_REQ_ID_STEP (1ULL << 1) + /** Maximum of max_pages received in init_out */ extern unsigned int fuse_max_pages_limit; @@ -490,7 +494,7 @@ struct fuse_iqueue { wait_queue_head_t waitq; /** The next unique request id */ - u64 reqctr; + u64 __percpu *reqctr; /** The list of pending requests */ struct list_head pending; @@ -1065,6 +1069,17 @@ static inline void fuse_sync_bucket_dec(struct fuse_sync_bucket *bucket) rcu_read_unlock(); } +/** + * Get the next unique ID for a request + */ +static inline u64 fuse_get_unique(struct fuse_iqueue *fiq) +{ + int step = FUSE_REQ_ID_STEP * (task_cpu(current)); + u64 cntr = this_cpu_inc_return(*fiq->reqctr); + + return cntr * FUSE_REQ_ID_STEP * NR_CPUS + step; +} + /** Device operations */ extern const struct file_operations fuse_dev_operations; @@ -1415,10 +1430,6 @@ int fuse_readdir(struct file *file, struct dir_context *ctx); */ unsigned int fuse_len_args(unsigned int numargs, struct fuse_arg *args); -/** - * Get the next unique ID for a request - */ -u64 fuse_get_unique(struct fuse_iqueue *fiq); void fuse_free_conn(struct fuse_conn *fc); /* dax.c */ diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c index e9db2cb8c150878634728685af0fa15e7ade628f..d2d850cca4c7bc3cd7158e773c5e602e15afe4e3 100644 --- a/fs/fuse/inode.c +++ b/fs/fuse/inode.c @@ -930,6 +930,7 @@ static void fuse_iqueue_init(struct fuse_iqueue *fiq, memset(fiq, 0, sizeof(struct fuse_iqueue)); spin_lock_init(&fiq->lock); init_waitqueue_head(&fiq->waitq); + fiq->reqctr = alloc_percpu(u64); INIT_LIST_HEAD(&fiq->pending); INIT_LIST_HEAD(&fiq->interrupts); fiq->forget_list_tail = &fiq->forget_list_head; @@ -938,6 +939,11 @@ static void fuse_iqueue_init(struct fuse_iqueue *fiq, fiq->priv = priv; } +static void fuse_iqueue_destroy(struct fuse_iqueue *fiq) +{ + free_percpu(fiq->reqctr); +} + void fuse_pqueue_init(struct fuse_pqueue *fpq) { unsigned int i; @@ -994,6 +1000,7 @@ static void delayed_release(struct rcu_head *p) struct fuse_conn *fc = container_of(p, struct fuse_conn, rcu); fuse_uring_destruct(fc); + fuse_iqueue_destroy(&fc->iq); put_user_ns(fc->user_ns); fc->release(fc);
No need to take lock, we can have that per cpu and add in the current cpu as offset. fuse-io-uring and virtiofs especially benefit from it as they don't need the fiq lock at all. Signed-off-by: Bernd Schubert <bschubert@ddn.com> --- fs/fuse/dev.c | 24 +++--------------------- fs/fuse/fuse_dev_i.h | 4 ---- fs/fuse/fuse_i.h | 21 ++++++++++++++++----- fs/fuse/inode.c | 7 +++++++ 4 files changed, 26 insertions(+), 30 deletions(-)