Re: [rds-devel] [PATCH RFC RFC] rds: Use NETDEV_UNREGISTER in rds_tcp_dev_event() (then kill NETDEV_UNREGISTER_FINAL)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Sowmini,

A little nit below. And some spelling issues in existing commentary you can consider fixing, since you reshuffle this file considerable.


Thxs, Håkon


> On 18 Mar 2018, at 21:45, Sowmini Varadhan <sowmini.varadhan@xxxxxxxxxx> wrote:
> 
> On (03/18/18 00:55), Kirill Tkhai wrote:
>> 
>> I just want to make rds not using NETDEV_UNREGISTER_FINAL. If there is
>> another solution to do that, I'm not again that.
> 
> The patch below takes care of this. I've done some preliminary testing,
> and I'll send it upstream after doing additional self-review/testing.
> Please also take a look, if you can, to see if I missed something.
> 
> Thanks for the input,
> 
> --Sowmini
> -------------------------------patch follows--------------------------------
> 
> diff --git a/net/rds/tcp.c b/net/rds/tcp.c
> index 08ea9cd..87c2643 100644
> --- a/net/rds/tcp.c
> +++ b/net/rds/tcp.c
> @@ -485,40 +485,6 @@ static __net_init int rds_tcp_init_net(struct net *net)
> 	return err;
> }
> 
> -static void __net_exit rds_tcp_exit_net(struct net *net)
> -{
> -	struct rds_tcp_net *rtn = net_generic(net, rds_tcp_netid);
> -
> -	if (rtn->rds_tcp_sysctl)
> -		unregister_net_sysctl_table(rtn->rds_tcp_sysctl);
> -
> -	if (net != &init_net && rtn->ctl_table)
> -		kfree(rtn->ctl_table);
> -
> -	/* If rds_tcp_exit_net() is called as a result of netns deletion,
> -	 * the rds_tcp_kill_sock() device notifier would already have cleaned
> -	 * up the listen socket, thus there is no work to do in this function.
> -	 *
> -	 * If rds_tcp_exit_net() is called as a result of module unload,
> -	 * i.e., due to rds_tcp_exit() -> unregister_pernet_subsys(), then
> -	 * we do need to clean up the listen socket here.
> -	 */
> -	if (rtn->rds_tcp_listen_sock) {
> -		struct socket *lsock = rtn->rds_tcp_listen_sock;
> -
> -		rtn->rds_tcp_listen_sock = NULL;
> -		rds_tcp_listen_stop(lsock, &rtn->rds_tcp_accept_w);
> -	}
> -}
> -
> -static struct pernet_operations rds_tcp_net_ops = {
> -	.init = rds_tcp_init_net,
> -	.exit = rds_tcp_exit_net,
> -	.id = &rds_tcp_netid,
> -	.size = sizeof(struct rds_tcp_net),
> -	.async = true,
> -};
> -
> static void rds_tcp_kill_sock(struct net *net)
> {
> 	struct rds_tcp_connection *tc, *_tc;
> @@ -546,40 +512,38 @@ static void rds_tcp_kill_sock(struct net *net)
> 		rds_conn_destroy(tc->t_cpath->cp_conn);
> }
> 
> -void *rds_tcp_listen_sock_def_readable(struct net *net)
> +static void __net_exit rds_tcp_exit_net(struct net *net)
> {
> 	struct rds_tcp_net *rtn = net_generic(net, rds_tcp_netid);
> -	struct socket *lsock = rtn->rds_tcp_listen_sock;
> 
> -	if (!lsock)
> -		return NULL;
> +	rds_tcp_kill_sock(net);
> 
> -	return lsock->sk->sk_user_data;
> +	if (rtn->rds_tcp_sysctl)
> +		unregister_net_sysctl_table(rtn->rds_tcp_sysctl);
> +
> +	if (net != &init_net && rtn->ctl_table)
> +		kfree(rtn->ctl_table);

Well, this comes from the existing code, but as pointed out by Linus recently, kfree() handles NULL pointers. So, if rtn->ctl_table is NULL most likely, the code is OK _if_ you add an unlikely() around the if-clause. Otherwise, the “ && rtn->ctl_table” can simply be removed.

> }
> 
> -static int rds_tcp_dev_event(struct notifier_block *this,
> -			     unsigned long event, void *ptr)
> +static struct pernet_operations rds_tcp_net_ops = {
> +	.init = rds_tcp_init_net,
> +	.exit = rds_tcp_exit_net,
> +	.id = &rds_tcp_netid,
> +	.size = sizeof(struct rds_tcp_net),
> +	.async = true,
> +};
> +
> +void *rds_tcp_listen_sock_def_readable(struct net *net)
> {
> -	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
> +	struct rds_tcp_net *rtn = net_generic(net, rds_tcp_netid);
> +	struct socket *lsock = rtn->rds_tcp_listen_sock;
> 
> -	/* rds-tcp registers as a pernet subys, so the ->exit will only
> -	 * get invoked after network acitivity has quiesced. We need to
> -	 * clean up all sockets  to quiesce network activity, and use
> -	 * the unregistration of the per-net loopback device as a trigger
> -	 * to start that cleanup.
> -	 */
> -	if (event == NETDEV_UNREGISTER_FINAL &&
> -	    dev->ifindex == LOOPBACK_IFINDEX)
> -		rds_tcp_kill_sock(dev_net(dev));
> +	if (!lsock)
> +		return NULL;
> 
> -	return NOTIFY_DONE;
> +	return lsock->sk->sk_user_data;
> }
> 
> -static struct notifier_block rds_tcp_dev_notifier = {
> -	.notifier_call        = rds_tcp_dev_event,
> -	.priority = -10, /* must be called after other network notifiers */
> -};
> -
> /* when sysctl is used to modify some kernel socket parameters,this

s/when/When/
s/parameters,this/parameters, this/

Well, not part of your commit.


>  * function  resets the RDS connections in that netns  so that we can

Two double spaces incidents above

Not part of your commit


>  * restart with new parameters.  The assumption is that such reset
> @@ -625,9 +589,7 @@ static void rds_tcp_exit(void)
> 	rds_tcp_set_unloading();
> 	synchronize_rcu();
> 	rds_info_deregister_func(RDS_INFO_TCP_SOCKETS, rds_tcp_tc_info);
> -	unregister_pernet_subsys(&rds_tcp_net_ops);
> -	if (unregister_netdevice_notifier(&rds_tcp_dev_notifier))
> -		pr_warn("could not unregister rds_tcp_dev_notifier\n");
> +	unregister_pernet_device(&rds_tcp_net_ops);
> 	rds_tcp_destroy_conns();
> 	rds_trans_unregister(&rds_tcp_transport);
> 	rds_tcp_recv_exit();
> @@ -651,24 +613,17 @@ static int rds_tcp_init(void)
> 	if (ret)
> 		goto out_slab;
> 
> -	ret = register_pernet_subsys(&rds_tcp_net_ops);
> +	ret = register_pernet_device(&rds_tcp_net_ops);
> 	if (ret)
> 		goto out_recv;
> 
> -	ret = register_netdevice_notifier(&rds_tcp_dev_notifier);
> -	if (ret) {
> -		pr_warn("could not register rds_tcp_dev_notifier\n");
> -		goto out_pernet;
> -	}
> -
> 	rds_trans_register(&rds_tcp_transport);
> 
> 	rds_info_register_func(RDS_INFO_TCP_SOCKETS, rds_tcp_tc_info);
> 
> 	goto out;
> 
> -out_pernet:
> -	unregister_pernet_subsys(&rds_tcp_net_ops);
> +	unregister_pernet_device(&rds_tcp_net_ops);
> out_recv:
> 	rds_tcp_recv_exit();
> out_slab:
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Photo]     [Yosemite News]     [Yosemite Photos]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux