Re: [patch net-next] net: core: introduce mini_Qdisc and eliminate usage of tp->q for clsact fastpath

[Jiri Pirko <jiri@resnulli.us>]

Tue, Oct 24, 2017 at 12:50:21PM CEST, daniel@iogearbox.net wrote:
>On 10/23/2017 11:28 PM, Jiri Pirko wrote:
>[...]
>> diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
>> index 031dffd..c7ddbdb 100644
>> --- a/include/net/sch_generic.h
>> +++ b/include/net/sch_generic.h
>> @@ -143,6 +143,36 @@ static inline int qdisc_avail_bulklimit(const struct netdev_queue *txq)
>>   #endif
>>   }
>> 
>> +/* Mini Qdisc serves for specific needs of ingress/clsact Qdisc.
>> + * The fast path only needs to access filter list and to update stats
>> + */
>> +struct mini_Qdisc {
>> +	struct tcf_proto __rcu *filter_list;
>> +	struct gnet_stats_basic_cpu __percpu *cpu_bstats;
>> +	struct gnet_stats_queue	__percpu *cpu_qstats;
>> +	struct mini_Qdisc __rcu **p_miniq;
>> +};
>> +
>> +static inline void mini_qdisc_init(struct mini_Qdisc *miniq,
>> +				   struct Qdisc *qdisc,
>> +				   struct mini_Qdisc __rcu **p_miniq)
>> +{
>> +	miniq->cpu_bstats = qdisc->cpu_bstats;
>> +	miniq->cpu_qstats = qdisc->cpu_qstats;
>> +	miniq->p_miniq = p_miniq;
>> +}
>> +
>> +static inline void mini_qdisc_enable(struct mini_Qdisc *miniq)
>> +{
>> +	rcu_assign_pointer(*miniq->p_miniq, miniq);
>> +}
>> +
>> +static inline void mini_qdisc_disable(struct mini_Qdisc *miniq)
>> +{
>> +	RCU_INIT_POINTER(*miniq->p_miniq, NULL);
>> +	rcu_barrier();
>
>Can you add a comment against which call_rcu() above barrier
>protects against?

Will do.

>
>> +}
>> +
>>   struct Qdisc_class_ops {
>>   	/* Child qdisc manipulation */
>>   	struct netdev_queue *	(*select_queue)(struct Qdisc *, struct tcmsg *);
>> @@ -259,9 +289,13 @@ struct qdisc_skb_cb {
>>   	unsigned char		data[QDISC_CB_PRIV_LEN];
>>   };
>> 
>> +typedef void tcf_chain_change_empty_t(struct tcf_proto __rcu **p_filter_chain,
>> +				      bool empty);
>> +
>>   struct tcf_chain {
>>   	struct tcf_proto __rcu *filter_chain;
>>   	struct tcf_proto __rcu **p_filter_chain;
>> +	tcf_chain_change_empty_t *chain_change_empty;
>>   	struct list_head list;
>>   	struct tcf_block *block;
>>   	u32 index; /* chain index */
>> @@ -605,6 +639,12 @@ static inline void qdisc_bstats_cpu_update(struct Qdisc *sch,
>>   	bstats_cpu_update(this_cpu_ptr(sch->cpu_bstats), skb);
>>   }
>> 
>> +static inline void mini_qdisc_bstats_cpu_update(struct mini_Qdisc *miniq,
>> +						const struct sk_buff *skb)
>> +{
>> +	bstats_cpu_update(this_cpu_ptr(miniq->cpu_bstats), skb);
>> +}
>> +
>>   static inline void qdisc_bstats_update(struct Qdisc *sch,
>>   				       const struct sk_buff *skb)
>>   {
>> @@ -648,6 +688,11 @@ static inline void qdisc_qstats_cpu_drop(struct Qdisc *sch)
>>   	this_cpu_inc(sch->cpu_qstats->drops);
>>   }
>> 
>> +static inline void mini_qdisc_qstats_cpu_drop(struct mini_Qdisc *miniq)
>> +{
>> +	this_cpu_inc(miniq->cpu_qstats->drops);
>> +}
>> +
>>   static inline void qdisc_qstats_overlimit(struct Qdisc *sch)
>>   {
>>   	sch->qstats.overlimits++;
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 24ac908..b4a5812 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -3274,14 +3274,16 @@ EXPORT_SYMBOL(dev_loopback_xmit);
>>   static struct sk_buff *
>>   sch_handle_egress(struct sk_buff *skb, int *ret, struct net_device *dev)
>>   {
>> -	struct tcf_proto *cl = rcu_dereference_bh(dev->egress_cl_list);
>> +	struct mini_Qdisc *miniq = rcu_dereference_bh(skb->dev->miniq_egress);
>
>We already have dev passed here, so lets use it as done previously.

Oops. Will do.


>
>>   	struct tcf_result cl_res;
>> +	struct tcf_proto *cl;
>> 
>> -	if (!cl)
>> +	if (!miniq)
>>   		return skb;
>> +	cl = rcu_dereference_bh(miniq->filter_list);
>
>This one still has two RCU dereferences instead of just one. Could
>we bind the lifetime of the miniq 1:1 to the filter_list head such
>that we can then also get rid of the 2nd rcu_dereference_bh() and
>piggy-back on the first one for the filter_list there, thus we push
>this into control slow-path instead?

The miniq is not assigned (skb->dev->miniq_egress == NULL) in case the
miniq->filter_list is empty. That is ensured by the slow-path and what
is why I don't check cl == NULL here.

I was thinking how to avoid the second rcu_dereference, but I was not
able to achieve that.

I don't get what you mean by "piggy-back" here. Could you please
elaborate a bit more?