Re: [PATCH 1/5 net-next] inet: replace ->bind_conflict with ->rcv_saddr_equal

Wed, 21 Dec 2016 07:17:47 -0800

On Wed, Dec 21, 2016 at 10:06 AM, Hannes Frederic Sowa <han...@stressinduktion.org> wrote: 
On Tue, 2016-12-20 at 15:07 -0500, Josef Bacik wrote:
The only difference between inet6_csk_bind_conflict and inet_csk_bind_conflict is how they check the rcv_saddr. Since we want to be able to check the saddr in other places just drop the protocol specific ->bind_conflict and replace it with ->rcv_saddr_equal, then make inet_csk_bind_conflict the one true bind conflict 
 function.

 Signed-off-by: Josef Bacik <jba...@fb.com>






 ---
  include/net/inet6_connection_sock.h |  5 -----
  include/net/inet_connection_sock.h  |  9 +++------
  net/dccp/ipv4.c                     |  3 ++-
  net/dccp/ipv6.c                     |  2 +-
  net/ipv4/inet_connection_sock.c     | 22 +++++++-------------
  net/ipv4/tcp_ipv4.c                 |  3 ++-
  net/ipv4/udp.c                      |  1 +
net/ipv6/inet6_connection_sock.c | 40 ------------------------------------- 
  net/ipv6/tcp_ipv6.c                 |  4 ++--
  9 files changed, 18 insertions(+), 71 deletions(-)
diff --git a/include/net/inet6_connection_sock.h b/include/net/inet6_connection_sock.h 
 index 3212b39..8ec87b6 100644
 --- a/include/net/inet6_connection_sock.h
 +++ b/include/net/inet6_connection_sock.h
 @@ -15,16 +15,11 @@

  #include <linux/types.h>

 -struct inet_bind_bucket;
  struct request_sock;
  struct sk_buff;
  struct sock;
  struct sockaddr;

 -int inet6_csk_bind_conflict(const struct sock *sk,
 -                          const struct inet_bind_bucket *tb, bool relax,
 -                          bool soreuseport_ok);
 -
struct dst_entry *inet6_csk_route_req(const struct sock *sk, struct flowi6 *fl6, 
                                      const struct request_sock *req, u8 proto);
diff --git a/include/net/inet_connection_sock.h b/include/net/inet_connection_sock.h 
 index ec0479a..9cd43c5 100644
 --- a/include/net/inet_connection_sock.h
 +++ b/include/net/inet_connection_sock.h
 @@ -62,9 +62,9 @@ struct inet_connection_sock_af_ops {
                                char __user *optval, int __user *optlen);
  #endif
        void        (*addr2sockaddr)(struct sock *sk, struct sockaddr *);
 -      int         (*bind_conflict)(const struct sock *sk,
 -                                   const struct inet_bind_bucket *tb,
 -                                   bool relax, bool soreuseport_ok);
 +      int         (*rcv_saddr_equal)(const struct sock *sk1,
 +                                     const struct sock *sk2,
 +                                     bool match_wildcard);
        void        (*mtu_reduced)(struct sock *sk);
  };




The patch looks as a nice code cleanup already!
Have you looked if we can simply have one rcv_saddr_equal for both ipv4 
and ipv6 that e.g. uses sk->sk_family instead of function pointers?
This could give us even more possibilities to remove some indirect
functions calls and thus might relieve some cycles?
I was going to do that but I'm not familiar enough with how sockets work to be comfortable. My main concern is we have the ipv6_only() check on a socket, which seems to indicate to me that you can have a socket that can do both ipv4/ipv6, so what if we're specifically going through the ipv6 code, but we aren't ipv6_only() and we end up doing the ipv4 address compare when we really need to do the ipv6 address compare? If this can't happen (and honestly as I type it out it sounds crazier than it did in my head) then yeah I'll totally do that as well and we can just have a global function without the protocol specific callbacks, but I need you or somebody to tell me I'm crazy and that it would be ok to have it all in one function. Thanks, 

Josef

Reply via email to