| Message ID | 20240223214003.17369-5-kuniyu@amazon.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | af_unix: Rework GC. | expand |
On Fri, 2024-02-23 at 13:39 -0800, Kuniyuki Iwashima wrote: > diff --git a/net/unix/garbage.c b/net/unix/garbage.c > index 96d0b1db3638..e8fe08796d02 100644 > --- a/net/unix/garbage.c > +++ b/net/unix/garbage.c > @@ -148,6 +148,7 @@ static void unix_free_vertices(struct scm_fp_list *fpl) > } > > DEFINE_SPINLOCK(unix_gc_lock); > +unsigned int unix_tot_inflight; > > void unix_add_edges(struct scm_fp_list *fpl, struct unix_sock *receiver) > { > @@ -172,7 +173,10 @@ void unix_add_edges(struct scm_fp_list *fpl, struct unix_sock *receiver) > unix_add_edge(fpl, edge); > } while (i < fpl->count_unix); > > + WRITE_ONCE(unix_tot_inflight, unix_tot_inflight + fpl->count_unix); > out: > + WRITE_ONCE(fpl->user->unix_inflight, fpl->user->unix_inflight + fpl->count); I'm unsure if later patches will shed some light, but why the above statement is placed _after_ the 'out' label? fpl->count will be 0 in such path, and the updated not needed. Why don't you place it before the mentioned label? > + > spin_unlock(&unix_gc_lock); > > fpl->inflight = true; > @@ -195,7 +199,10 @@ void unix_del_edges(struct scm_fp_list *fpl) > unix_del_edge(fpl, edge); > } while (i < fpl->count_unix); > > + WRITE_ONCE(unix_tot_inflight, unix_tot_inflight - fpl->count_unix); > out: > + WRITE_ONCE(fpl->user->unix_inflight, fpl->user->unix_inflight - fpl->count); Same question here. Thanks! Paolo
From: Paolo Abeni <pabeni@redhat.com> Date: Tue, 27 Feb 2024 11:47:23 +0100 > On Fri, 2024-02-23 at 13:39 -0800, Kuniyuki Iwashima wrote: > > diff --git a/net/unix/garbage.c b/net/unix/garbage.c > > index 96d0b1db3638..e8fe08796d02 100644 > > --- a/net/unix/garbage.c > > +++ b/net/unix/garbage.c > > @@ -148,6 +148,7 @@ static void unix_free_vertices(struct scm_fp_list *fpl) > > } > > > > DEFINE_SPINLOCK(unix_gc_lock); > > +unsigned int unix_tot_inflight; > > > > void unix_add_edges(struct scm_fp_list *fpl, struct unix_sock *receiver) > > { > > @@ -172,7 +173,10 @@ void unix_add_edges(struct scm_fp_list *fpl, struct unix_sock *receiver) > > unix_add_edge(fpl, edge); > > } while (i < fpl->count_unix); > > > > + WRITE_ONCE(unix_tot_inflight, unix_tot_inflight + fpl->count_unix); > > out: > > + WRITE_ONCE(fpl->user->unix_inflight, fpl->user->unix_inflight + fpl->count); > > I'm unsure if later patches will shed some light, but why the above > statement is placed _after_ the 'out' label? fpl->count will be 0 in > such path, and the updated not needed. Why don't you place it before > the mentioned label? fpl->count is the total number of fds in skb, and fpl->count_unix is the number of AF_UNIX fds. So, we could reach the out: label if we pass no AF_UNIX fd but other fds, then we count the number for each user to use in too_many_unix_fds(). Before this change, unix_inflight() and unix_notinflight() did the same but incremented/decremented one by one. > > > + > > spin_unlock(&unix_gc_lock); > > > > fpl->inflight = true; > > @@ -195,7 +199,10 @@ void unix_del_edges(struct scm_fp_list *fpl) > > unix_del_edge(fpl, edge); > > } while (i < fpl->count_unix); > > > > + WRITE_ONCE(unix_tot_inflight, unix_tot_inflight - fpl->count_unix); > > out: > > + WRITE_ONCE(fpl->user->unix_inflight, fpl->user->unix_inflight - fpl->count); > > Same question here. > > Thanks! > > Paolo
On Tue, 2024-02-27 at 18:34 -0800, Kuniyuki Iwashima wrote: > From: Paolo Abeni <pabeni@redhat.com> > Date: Tue, 27 Feb 2024 11:47:23 +0100 > > On Fri, 2024-02-23 at 13:39 -0800, Kuniyuki Iwashima wrote: > > > diff --git a/net/unix/garbage.c b/net/unix/garbage.c > > > index 96d0b1db3638..e8fe08796d02 100644 > > > --- a/net/unix/garbage.c > > > +++ b/net/unix/garbage.c > > > @@ -148,6 +148,7 @@ static void unix_free_vertices(struct scm_fp_list *fpl) > > > } > > > > > > DEFINE_SPINLOCK(unix_gc_lock); > > > +unsigned int unix_tot_inflight; > > > > > > void unix_add_edges(struct scm_fp_list *fpl, struct unix_sock *receiver) > > > { > > > @@ -172,7 +173,10 @@ void unix_add_edges(struct scm_fp_list *fpl, struct unix_sock *receiver) > > > unix_add_edge(fpl, edge); > > > } while (i < fpl->count_unix); > > > > > > + WRITE_ONCE(unix_tot_inflight, unix_tot_inflight + fpl->count_unix); > > > out: > > > + WRITE_ONCE(fpl->user->unix_inflight, fpl->user->unix_inflight + fpl->count); > > > > I'm unsure if later patches will shed some light, but why the above > > statement is placed _after_ the 'out' label? fpl->count will be 0 in > > such path, and the updated not needed. Why don't you place it before > > the mentioned label? > > fpl->count is the total number of fds in skb, and fpl->count_unix > is the number of AF_UNIX fds. Ah right you are! Sorry, I misread the variable name. This code looks good. Cheers, Paolo
diff --git a/net/unix/garbage.c b/net/unix/garbage.c index 96d0b1db3638..e8fe08796d02 100644 --- a/net/unix/garbage.c +++ b/net/unix/garbage.c @@ -148,6 +148,7 @@ static void unix_free_vertices(struct scm_fp_list *fpl) } DEFINE_SPINLOCK(unix_gc_lock); +unsigned int unix_tot_inflight; void unix_add_edges(struct scm_fp_list *fpl, struct unix_sock *receiver) { @@ -172,7 +173,10 @@ void unix_add_edges(struct scm_fp_list *fpl, struct unix_sock *receiver) unix_add_edge(fpl, edge); } while (i < fpl->count_unix); + WRITE_ONCE(unix_tot_inflight, unix_tot_inflight + fpl->count_unix); out: + WRITE_ONCE(fpl->user->unix_inflight, fpl->user->unix_inflight + fpl->count); + spin_unlock(&unix_gc_lock); fpl->inflight = true; @@ -195,7 +199,10 @@ void unix_del_edges(struct scm_fp_list *fpl) unix_del_edge(fpl, edge); } while (i < fpl->count_unix); + WRITE_ONCE(unix_tot_inflight, unix_tot_inflight - fpl->count_unix); out: + WRITE_ONCE(fpl->user->unix_inflight, fpl->user->unix_inflight - fpl->count); + spin_unlock(&unix_gc_lock); fpl->inflight = false; @@ -238,7 +245,6 @@ void unix_destroy_fpl(struct scm_fp_list *fpl) unix_free_vertices(fpl); } -unsigned int unix_tot_inflight; static LIST_HEAD(gc_candidates); static LIST_HEAD(gc_inflight_list); @@ -259,13 +265,8 @@ void unix_inflight(struct user_struct *user, struct file *filp) WARN_ON_ONCE(list_empty(&u->link)); } u->inflight++; - - /* Paired with READ_ONCE() in wait_for_unix_gc() */ - WRITE_ONCE(unix_tot_inflight, unix_tot_inflight + 1); } - WRITE_ONCE(user->unix_inflight, user->unix_inflight + 1); - spin_unlock(&unix_gc_lock); } @@ -282,13 +283,8 @@ void unix_notinflight(struct user_struct *user, struct file *filp) u->inflight--; if (!u->inflight) list_del_init(&u->link); - - /* Paired with READ_ONCE() in wait_for_unix_gc() */ - WRITE_ONCE(unix_tot_inflight, unix_tot_inflight - 1); } - WRITE_ONCE(user->unix_inflight, user->unix_inflight - 1); - spin_unlock(&unix_gc_lock); }
Currently, we track the number of inflight sockets in two variables. unix_tot_inflight is the total number of inflight AF_UNIX sockets on the host, and user->unix_inflight is the number of inflight fds per user. We update them one by one in unix_inflight(), which can be done once in batch. Also, sendmsg() could fail even after unix_inflight(), then we need to acquire unix_gc_lock only to decrement the counters. Let's bulk update the counters in unix_add_edges() and unix_del_edges(), which is called only for successfully passed fds. Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com> --- net/unix/garbage.c | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-)