Re: [net-next PATCH v4 11/16] net: sched: rcu'ify cls_rsvp

[John Fastabend <john.fastabend@gmail.com>]

On 09/10/2014 06:30 PM, Eric Dumazet wrote:
> On Wed, 2014-09-10 at 08:51 -0700, John Fastabend wrote:
>> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
>> ---
>>   net/sched/cls_rsvp.h |  157 ++++++++++++++++++++++++++++----------------------
>>   1 file changed, 89 insertions(+), 68 deletions(-)
>>
>> diff --git a/net/sched/cls_rsvp.h b/net/sched/cls_rsvp.h
>> index 1020e23..afa508b 100644
>> --- a/net/sched/cls_rsvp.h
>> +++ b/net/sched/cls_rsvp.h
>> @@ -70,31 +70,34 @@ struct rsvp_head {
>>   	u32			tmap[256/32];
>>   	u32			hgenerator;
>>   	u8			tgenerator;
>> -	struct rsvp_session	*ht[256];
>> +	struct rsvp_session __rcu *ht[256];
>> +	struct rcu_head		rcu;
>>   };
>>
>>   struct rsvp_session {
>> -	struct rsvp_session	*next;
>> -	__be32			dst[RSVP_DST_LEN];
>> -	struct tc_rsvp_gpi 	dpi;
>> -	u8			protocol;
>> -	u8			tunnelid;
>> +	struct rsvp_session __rcu	*next;
>> +	__be32				dst[RSVP_DST_LEN];
>> +	struct tc_rsvp_gpi		dpi;
>> +	u8				protocol;
>> +	u8				tunnelid;
>>   	/* 16 (src,sport) hash slots, and one wildcard source slot */
>> -	struct rsvp_filter	*ht[16 + 1];
>> +	struct rsvp_filter __rcu	*ht[16 + 1];
>> +	struct rcu_head			rcu;
>>   };
>>
>>
>>   struct rsvp_filter {
>> -	struct rsvp_filter	*next;
>> -	__be32			src[RSVP_DST_LEN];
>> -	struct tc_rsvp_gpi	spi;
>> -	u8			tunnelhdr;
>> +	struct rsvp_filter __rcu	*next;
>> +	__be32				src[RSVP_DST_LEN];
>> +	struct tc_rsvp_gpi		spi;
>> +	u8				tunnelhdr;
>>
>> -	struct tcf_result	res;
>> -	struct tcf_exts		exts;
>> +	struct tcf_result		res;
>> +	struct tcf_exts			exts;
>>
>> -	u32			handle;
>> -	struct rsvp_session	*sess;
>> +	u32				handle;
>> +	struct rsvp_session		*sess;
>> +	struct rcu_head			rcu;
>>   };
>>
>>   static inline unsigned int hash_dst(__be32 *dst, u8 protocol, u8 tunnelid)
>> @@ -128,7 +131,7 @@ static inline unsigned int hash_src(__be32 *src)
>>   static int rsvp_classify(struct sk_buff *skb, const struct tcf_proto *tp,
>>   			 struct tcf_result *res)
>>   {
>> -	struct rsvp_session **sht = ((struct rsvp_head *)tp->root)->ht;
>> +	struct rsvp_head *head = rcu_dereference_bh(tp->root);
>>   	struct rsvp_session *s;
>>   	struct rsvp_filter *f;
>>   	unsigned int h1, h2;
>> @@ -169,7 +172,8 @@ restart:
>>   	h1 = hash_dst(dst, protocol, tunnelid);
>>   	h2 = hash_src(src);
>>
>> -	for (s = sht[h1]; s; s = s->next) {
>> +	for (s = rcu_dereference_bh(head->ht[h1]); s;
>> +	     s = rcu_dereference_bh(s->next)) {
>>   		if (dst[RSVP_DST_LEN-1] == s->dst[RSVP_DST_LEN - 1] &&
>>   		    protocol == s->protocol &&
>>   		    !(s->dpi.mask &
>> @@ -181,7 +185,8 @@ restart:
>>   #endif
>>   		    tunnelid == s->tunnelid) {
>>
>> -			for (f = s->ht[h2]; f; f = f->next) {
>> +			for (f = rcu_dereference_bh(s->ht[h2]); f;
>> +			     f = rcu_dereference_bh(f->next)) {
>>   				if (src[RSVP_DST_LEN-1] == f->src[RSVP_DST_LEN - 1] &&
>>   				    !(f->spi.mask & (*(u32 *)(xprt + f->spi.offset) ^ f->spi.key))
>>   #if RSVP_DST_LEN == 4
>> @@ -205,7 +210,8 @@ matched:
>>   			}
>>
>>   			/* And wildcard bucket... */
>> -			for (f = s->ht[16]; f; f = f->next) {
>> +			for (f = rcu_dereference_bh(s->ht[16]); f;
>> +			     f = rcu_dereference_bh(f->next)) {
>>   				*res = f->res;
>>   				RSVP_APPLY_RESULT();
>>   				goto matched;
>> @@ -218,7 +224,7 @@ matched:
>>
>>   static unsigned long rsvp_get(struct tcf_proto *tp, u32 handle)
>>   {
>> -	struct rsvp_session **sht = ((struct rsvp_head *)tp->root)->ht;
>> +	struct rsvp_head *head = rtnl_dereference(tp->root);
>>   	struct rsvp_session *s;
>>   	struct rsvp_filter *f;
>>   	unsigned int h1 = handle & 0xFF;
>> @@ -227,8 +233,10 @@ static unsigned long rsvp_get(struct tcf_proto *tp, u32 handle)
>>   	if (h2 > 16)
>>   		return 0;
>>
>> -	for (s = sht[h1]; s; s = s->next) {
>> -		for (f = s->ht[h2]; f; f = f->next) {
>> +	for (s = rtnl_dereference(head->ht[h1]); s;
>> +	     s = rtnl_dereference(s->next)) {
>> +		for (f = rtnl_dereference(s->ht[h2]); f;
>> +		     f = rtnl_dereference(f->next)) {
>>   			if (f->handle == handle)
>>   				return (unsigned long)f;
>>   		}
>> @@ -246,7 +254,7 @@ static int rsvp_init(struct tcf_proto *tp)
>>
>>   	data = kzalloc(sizeof(struct rsvp_head), GFP_KERNEL);
>>   	if (data) {
>> -		tp->root = data;
>> +		rcu_assign_pointer(tp->root, data);
>>   		return 0;
>>   	}
>>   	return -ENOBUFS;
>> @@ -257,53 +265,55 @@ rsvp_delete_filter(struct tcf_proto *tp, struct rsvp_filter *f)
>>   {
>>   	tcf_unbind_filter(tp, &f->res);
>>   	tcf_exts_destroy(tp, &f->exts);
>> -	kfree(f);
>> +	kfree_rcu(f, rcu);
>>   }
>>
>>   static void rsvp_destroy(struct tcf_proto *tp)
>>   {
>> -	struct rsvp_head *data = xchg(&tp->root, NULL);
>> -	struct rsvp_session **sht;
>> +	struct rsvp_head *data = rtnl_dereference(tp->root);
>>   	int h1, h2;
>>
>>   	if (data == NULL)
>>   		return;
>>
>> -	sht = data->ht;
>> +	RCU_INIT_POINTER(tp->root, NULL);
>>
>>   	for (h1 = 0; h1 < 256; h1++) {
>>   		struct rsvp_session *s;
>>
>> -		while ((s = sht[h1]) != NULL) {
>> -			sht[h1] = s->next;
>> +		while ((s = rtnl_dereference(data->ht[h1])) != NULL) {
>> +			RCU_INIT_POINTER(data->ht[h1], s->next);
>>
>>   			for (h2 = 0; h2 <= 16; h2++) {
>>   				struct rsvp_filter *f;
>>
>> -				while ((f = s->ht[h2]) != NULL) {
>> -					s->ht[h2] = f->next;
>> +				while ((f = rtnl_dereference(s->ht[h2])) != NULL) {
>> +					rcu_assign_pointer(s->ht[h2], f->next);
>>   					rsvp_delete_filter(tp, f);
>>   				}
>>   			}
>> -			kfree(s);
>> +			kfree_rcu(s, rcu);
>>   		}
>>   	}
>> -	kfree(data);
>> +	RCU_INIT_POINTER(tp->root, NULL);
>
> Strange, you already did the RCU_INIT_POINTER(tp->root, NULL) before the
> for(h1 = 0; h1 < 256; h1++) loop
>

Yep, I'll drop this duplicate call.

>
>
>> +	kfree_rcu(data, rcu);
>>   }
>>
>>   static int rsvp_delete(struct tcf_proto *tp, unsigned long arg)
>>   {
>> -	struct rsvp_filter **fp, *f = (struct rsvp_filter *)arg;
>> +	struct rsvp_head *head = rtnl_dereference(tp->root);
>> +	struct rsvp_filter *nfp, *f = (struct rsvp_filter *)arg;
>> +	struct rsvp_filter __rcu **fp;
>>   	unsigned int h = f->handle;
>> -	struct rsvp_session **sp;
>> -	struct rsvp_session *s = f->sess;
>> +	struct rsvp_session __rcu **sp;
>> +	struct rsvp_session *nsp, *s = f->sess;
>>   	int i;
>>
>> -	for (fp = &s->ht[(h >> 8) & 0xFF]; *fp; fp = &(*fp)->next) {
>> -		if (*fp == f) {
>> -			tcf_tree_lock(tp);
>> +	fp = &s->ht[(h >> 8) & 0xFF];
>> +	for (nfp = rtnl_dereference(*fp); nfp;
>> +	     fp = &nfp->next, nfp = rtnl_dereference(*fp)) {
>> +		if (nfp == f) {
>>   			*fp = f->next;
>
> It seems you do not follow your own convention here ?
>
> RCU_INIT_POINTER(*fp, f->next);

converted both cases to RCU_INIT_POINTER().

Thanks.


-- 
John Fastabend         Intel Corporation