Message ID | 20200909181556.2945496-4-ncardwell@google.com |
---|---|
State | Changes Requested |
Delegated to: | BPF Maintainers |
Headers | show |
Series | tcp: increase flexibility of EBPF congestion control initialization | expand |
On Wed, Sep 09, 2020 at 02:15:55PM -0400, Neal Cardwell wrote: > Now that the previous patches ensure that all call sites for > tcp_set_congestion_control() want to initialize congestion control, we > can simplify tcp_set_congestion_control() by removing the reinit > argument and the code to support it. > > Signed-off-by: Neal Cardwell <ncardwell@google.com> > Acked-by: Yuchung Cheng <ycheng@google.com> > Acked-by: Kevin Yang <yyd@google.com> > Signed-off-by: Eric Dumazet <edumazet@google.com> > Cc: Lawrence Brakmo <brakmo@fb.com> > --- > include/net/tcp.h | 2 +- > net/core/filter.c | 3 +-- > net/ipv4/tcp.c | 2 +- > net/ipv4/tcp_cong.c | 11 ++--------- > 4 files changed, 5 insertions(+), 13 deletions(-) > > diff --git a/include/net/tcp.h b/include/net/tcp.h > index e85d564446c6..f857146c17a5 100644 > --- a/include/net/tcp.h > +++ b/include/net/tcp.h > @@ -1104,7 +1104,7 @@ void tcp_get_available_congestion_control(char *buf, size_t len); > void tcp_get_allowed_congestion_control(char *buf, size_t len); > int tcp_set_allowed_congestion_control(char *allowed); > int tcp_set_congestion_control(struct sock *sk, const char *name, bool load, > - bool reinit, bool cap_net_admin); > + bool cap_net_admin); > u32 tcp_slow_start(struct tcp_sock *tp, u32 acked); > void tcp_cong_avoid_ai(struct tcp_sock *tp, u32 w, u32 acked); > > diff --git a/net/core/filter.c b/net/core/filter.c > index b26c04924fa3..0bd0a97ee951 100644 > --- a/net/core/filter.c > +++ b/net/core/filter.c > @@ -4451,8 +4451,7 @@ static int _bpf_setsockopt(struct sock *sk, int level, int optname, > strncpy(name, optval, min_t(long, optlen, > TCP_CA_NAME_MAX-1)); > name[TCP_CA_NAME_MAX-1] = 0; > - ret = tcp_set_congestion_control(sk, name, false, > - true, true); > + ret = tcp_set_congestion_control(sk, name, false, true); > } else { > struct inet_connection_sock *icsk = inet_csk(sk); > struct tcp_sock *tp = tcp_sk(sk); > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c > index 7360d3db2b61..e58ab9db73ff 100644 > --- a/net/ipv4/tcp.c > +++ b/net/ipv4/tcp.c > @@ -3050,7 +3050,7 @@ static int do_tcp_setsockopt(struct sock *sk, int level, int optname, > name[val] = 0; > > lock_sock(sk); > - err = tcp_set_congestion_control(sk, name, true, true, > + err = tcp_set_congestion_control(sk, name, true, > ns_capable(sock_net(sk)->user_ns, > CAP_NET_ADMIN)); > release_sock(sk); > diff --git a/net/ipv4/tcp_cong.c b/net/ipv4/tcp_cong.c > index d18d7a1ce4ce..a9b0fb52a1ec 100644 > --- a/net/ipv4/tcp_cong.c > +++ b/net/ipv4/tcp_cong.c > @@ -341,7 +341,7 @@ int tcp_set_allowed_congestion_control(char *val) > * already initialized. > */ > int tcp_set_congestion_control(struct sock *sk, const char *name, bool load, > - bool reinit, bool cap_net_admin) > + bool cap_net_admin) > { > struct inet_connection_sock *icsk = inet_csk(sk); > const struct tcp_congestion_ops *ca; > @@ -365,15 +365,8 @@ int tcp_set_congestion_control(struct sock *sk, const char *name, bool load, > if (!ca) { > err = -ENOENT; > } else if (!load) { nit. I think this "else if (!load)" case can be completely removed and simply allow it to fall through to the last "else { tcp_reinit_congestion_control(sk, ca); }" . > - const struct tcp_congestion_ops *old_ca = icsk->icsk_ca_ops; > - > if (bpf_try_module_get(ca, ca->owner)) { > - if (reinit) { > - tcp_reinit_congestion_control(sk, ca); > - } else { > - icsk->icsk_ca_ops = ca; > - bpf_module_put(old_ca, old_ca->owner); > - } > + tcp_reinit_congestion_control(sk, ca); > } else { > err = -EBUSY; > } > -- > 2.28.0.526.ge36021eeef-goog >
On Wed, Sep 9, 2020 at 8:29 PM Martin KaFai Lau <kafai@fb.com> wrote: > > On Wed, Sep 09, 2020 at 02:15:55PM -0400, Neal Cardwell wrote: > > Now that the previous patches ensure that all call sites for > > tcp_set_congestion_control() want to initialize congestion control, we > > can simplify tcp_set_congestion_control() by removing the reinit > > argument and the code to support it. > > > > Signed-off-by: Neal Cardwell <ncardwell@google.com> > > Acked-by: Yuchung Cheng <ycheng@google.com> > > Acked-by: Kevin Yang <yyd@google.com> > > Signed-off-by: Eric Dumazet <edumazet@google.com> > > Cc: Lawrence Brakmo <brakmo@fb.com> > > --- > > include/net/tcp.h | 2 +- > > net/core/filter.c | 3 +-- > > net/ipv4/tcp.c | 2 +- > > net/ipv4/tcp_cong.c | 11 ++--------- > > 4 files changed, 5 insertions(+), 13 deletions(-) > > > > diff --git a/include/net/tcp.h b/include/net/tcp.h > > index e85d564446c6..f857146c17a5 100644 > > --- a/include/net/tcp.h > > +++ b/include/net/tcp.h > > @@ -1104,7 +1104,7 @@ void tcp_get_available_congestion_control(char *buf, size_t len); > > void tcp_get_allowed_congestion_control(char *buf, size_t len); > > int tcp_set_allowed_congestion_control(char *allowed); > > int tcp_set_congestion_control(struct sock *sk, const char *name, bool load, > > - bool reinit, bool cap_net_admin); > > + bool cap_net_admin); > > u32 tcp_slow_start(struct tcp_sock *tp, u32 acked); > > void tcp_cong_avoid_ai(struct tcp_sock *tp, u32 w, u32 acked); > > > > diff --git a/net/core/filter.c b/net/core/filter.c > > index b26c04924fa3..0bd0a97ee951 100644 > > --- a/net/core/filter.c > > +++ b/net/core/filter.c > > @@ -4451,8 +4451,7 @@ static int _bpf_setsockopt(struct sock *sk, int level, int optname, > > strncpy(name, optval, min_t(long, optlen, > > TCP_CA_NAME_MAX-1)); > > name[TCP_CA_NAME_MAX-1] = 0; > > - ret = tcp_set_congestion_control(sk, name, false, > > - true, true); > > + ret = tcp_set_congestion_control(sk, name, false, true); > > } else { > > struct inet_connection_sock *icsk = inet_csk(sk); > > struct tcp_sock *tp = tcp_sk(sk); > > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c > > index 7360d3db2b61..e58ab9db73ff 100644 > > --- a/net/ipv4/tcp.c > > +++ b/net/ipv4/tcp.c > > @@ -3050,7 +3050,7 @@ static int do_tcp_setsockopt(struct sock *sk, int level, int optname, > > name[val] = 0; > > > > lock_sock(sk); > > - err = tcp_set_congestion_control(sk, name, true, true, > > + err = tcp_set_congestion_control(sk, name, true, > > ns_capable(sock_net(sk)->user_ns, > > CAP_NET_ADMIN)); > > release_sock(sk); > > diff --git a/net/ipv4/tcp_cong.c b/net/ipv4/tcp_cong.c > > index d18d7a1ce4ce..a9b0fb52a1ec 100644 > > --- a/net/ipv4/tcp_cong.c > > +++ b/net/ipv4/tcp_cong.c > > @@ -341,7 +341,7 @@ int tcp_set_allowed_congestion_control(char *val) > > * already initialized. > > */ > > int tcp_set_congestion_control(struct sock *sk, const char *name, bool load, > > - bool reinit, bool cap_net_admin) > > + bool cap_net_admin) > > { > > struct inet_connection_sock *icsk = inet_csk(sk); > > const struct tcp_congestion_ops *ca; > > @@ -365,15 +365,8 @@ int tcp_set_congestion_control(struct sock *sk, const char *name, bool load, > > if (!ca) { > > err = -ENOENT; > > } else if (!load) { > nit. > > I think this "else if (!load)" case can be completely removed and simply > allow it to fall through to the last > "else { tcp_reinit_congestion_control(sk, ca); }" . Thanks, Martin. That is a very nice observation, and I think you are right that we can make the additional refactor/simplification/clean-up that you mention, without changing behavior. For clarity I think that would be nice to have as a separate follow-on commit. Are you interested in posting a follow-on patch for that, since it's your nice idea? thanks, neal
diff --git a/include/net/tcp.h b/include/net/tcp.h index e85d564446c6..f857146c17a5 100644 --- a/include/net/tcp.h +++ b/include/net/tcp.h @@ -1104,7 +1104,7 @@ void tcp_get_available_congestion_control(char *buf, size_t len); void tcp_get_allowed_congestion_control(char *buf, size_t len); int tcp_set_allowed_congestion_control(char *allowed); int tcp_set_congestion_control(struct sock *sk, const char *name, bool load, - bool reinit, bool cap_net_admin); + bool cap_net_admin); u32 tcp_slow_start(struct tcp_sock *tp, u32 acked); void tcp_cong_avoid_ai(struct tcp_sock *tp, u32 w, u32 acked); diff --git a/net/core/filter.c b/net/core/filter.c index b26c04924fa3..0bd0a97ee951 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -4451,8 +4451,7 @@ static int _bpf_setsockopt(struct sock *sk, int level, int optname, strncpy(name, optval, min_t(long, optlen, TCP_CA_NAME_MAX-1)); name[TCP_CA_NAME_MAX-1] = 0; - ret = tcp_set_congestion_control(sk, name, false, - true, true); + ret = tcp_set_congestion_control(sk, name, false, true); } else { struct inet_connection_sock *icsk = inet_csk(sk); struct tcp_sock *tp = tcp_sk(sk); diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c index 7360d3db2b61..e58ab9db73ff 100644 --- a/net/ipv4/tcp.c +++ b/net/ipv4/tcp.c @@ -3050,7 +3050,7 @@ static int do_tcp_setsockopt(struct sock *sk, int level, int optname, name[val] = 0; lock_sock(sk); - err = tcp_set_congestion_control(sk, name, true, true, + err = tcp_set_congestion_control(sk, name, true, ns_capable(sock_net(sk)->user_ns, CAP_NET_ADMIN)); release_sock(sk); diff --git a/net/ipv4/tcp_cong.c b/net/ipv4/tcp_cong.c index d18d7a1ce4ce..a9b0fb52a1ec 100644 --- a/net/ipv4/tcp_cong.c +++ b/net/ipv4/tcp_cong.c @@ -341,7 +341,7 @@ int tcp_set_allowed_congestion_control(char *val) * already initialized. */ int tcp_set_congestion_control(struct sock *sk, const char *name, bool load, - bool reinit, bool cap_net_admin) + bool cap_net_admin) { struct inet_connection_sock *icsk = inet_csk(sk); const struct tcp_congestion_ops *ca; @@ -365,15 +365,8 @@ int tcp_set_congestion_control(struct sock *sk, const char *name, bool load, if (!ca) { err = -ENOENT; } else if (!load) { - const struct tcp_congestion_ops *old_ca = icsk->icsk_ca_ops; - if (bpf_try_module_get(ca, ca->owner)) { - if (reinit) { - tcp_reinit_congestion_control(sk, ca); - } else { - icsk->icsk_ca_ops = ca; - bpf_module_put(old_ca, old_ca->owner); - } + tcp_reinit_congestion_control(sk, ca); } else { err = -EBUSY; }