Re: [PATCH RFC 1/2] tcp: add generic netlink support for tcp_metrics

[Eric Dumazet <eric.dumazet@gmail.com>]

On Thu, 2012-08-23 at 18:01 +0300, Julian Anastasov wrote:
> 	Add support for genl "tcp_metrics". No locking
> is changed, only that now we can unlink and delete
> entries after grace period. We implement get/del for
> single entry and dump to support show/flush filtering
> in user space.
> 

Very nice, thanks !

> +static int tcp_metrics_nl_cmd_del(struct sk_buff *skb, struct genl_info *info)
> +{
> +	struct tcpm_hash_bucket *hb;
> +	struct tcp_metrics_block *tm;
> +	struct tcp_metrics_block __rcu **pp;
> +	struct inetpeer_addr addr;
> +	unsigned int hash;
> +	struct net *net = genl_info_net(info);
> +	int ret;
> +
> +	ret = parse_nl_addr(info, &addr, &hash, 1);
> +	if (ret < 0)
> +		return ret;
> +	/* Flush all ? */
> +	if (ret > 0) {
> +		unsigned int max_rows = 1U << net->ipv4.tcp_metrics_hash_log;
> +		unsigned int sync_count = 0;
> +		unsigned int row;
> +
> +		hb = net->ipv4.tcp_metrics_hash;
> +		for (row = 0; row < max_rows; row++, hb++) {
> +			spin_lock_bh(&tcp_metrics_lock);
> +			tm = deref_locked_genl(hb->chain);
> +			if (tm)
> +				rcu_assign_pointer(hb->chain, NULL);
> +			spin_unlock_bh(&tcp_metrics_lock);
> +			while (tm) {
> +				struct tcp_metrics_block *next;
> +
> +				next = deref_genl(tm->tcpm_next);
> +				kfree_rcu(tm, rcu_head);
> +				if (!((++sync_count) & 2047))
> +					synchronize_rcu();
> +				tm = next;
> +			}
> +		}
> +		return 0;
> +	}
> +
> +	hash = hash_32(hash, net->ipv4.tcp_metrics_hash_log);
> +	hb = net->ipv4.tcp_metrics_hash + hash;
> +	pp = &hb->chain;
> +	spin_lock_bh(&tcp_metrics_lock);
> +	for (tm = deref_locked_genl(*pp); tm;
> +	     pp = &tm->tcpm_next, tm = deref_locked_genl(*pp)) {
> +		if (addr_same(&tm->tcpm_addr, &addr)) {
> +			rcu_assign_pointer(*pp, tm->tcpm_next);

Hmm, try "make C=2 net/ipv4/tcp_metrics.o"

with :

CONFIG_SPARSE_RCU_POINTER=y


I guess you can use plain " *pp = tm->tcpm_next;"

> +			break;
> +		}
> +	}
> +	spin_unlock_bh(&tcp_metrics_lock);
> +	if (!tm)
> +		return -ESRCH;
> +	kfree_rcu(tm, rcu_head);
> +	return 0;
> +}
> +

Could you split this in two functions, adding tcp_metrics_flush_all() ?