Re: [patch net-next 06/34] net: core: use dev->ingress_queue instead of tp->q

[Jiri Pirko <jiri@resnulli.us>]

Mon, Oct 16, 2017 at 10:20:04PM CEST, daniel@iogearbox.net wrote:
>On 10/15/2017 08:45 AM, Jiri Pirko wrote:
>> Sun, Oct 15, 2017 at 01:18:54AM CEST, daniel@iogearbox.net wrote:
>> > On 10/13/2017 08:30 AM, Jiri Pirko wrote:
>> > > Thu, Oct 12, 2017 at 11:45:43PM CEST, daniel@iogearbox.net wrote:
>> > > > On 10/12/2017 07:17 PM, Jiri Pirko wrote:
>> > > > > From: Jiri Pirko <jiri@mellanox.com>
>> > > > > 
>> > > > > In sch_handle_egress and sch_handle_ingress, don't use tp->q and use
>> > > > > dev->ingress_queue which stores the same pointer instead.
>> > > > > 
>> > > > > Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>> > > > > ---
>> > > > >     net/core/dev.c | 21 +++++++++++++++------
>> > > > >     1 file changed, 15 insertions(+), 6 deletions(-)
>> > > > > 
>> > > > > diff --git a/net/core/dev.c b/net/core/dev.c
>> > > > > index fcddccb..cb9e5e5 100644
>> > > > > --- a/net/core/dev.c
>> > > > > +++ b/net/core/dev.c
>> > > > > @@ -3273,14 +3273,18 @@ EXPORT_SYMBOL(dev_loopback_xmit);
>> > > > >     static struct sk_buff *
>> > > > >     sch_handle_egress(struct sk_buff *skb, int *ret, struct net_device *dev)
>> > > > >     {
>> > > > > +	struct netdev_queue *netdev_queue =
>> > > > > +				rcu_dereference_bh(dev->ingress_queue);
>> > > > >     	struct tcf_proto *cl = rcu_dereference_bh(dev->egress_cl_list);
>> > > > >     	struct tcf_result cl_res;
>> > > > > +	struct Qdisc *q;
>> > > > > 
>> > > > > -	if (!cl)
>> > > > > +	if (!cl || !netdev_queue)
>> > > > >     		return skb;
>> > > > > +	q = netdev_queue->qdisc;
>> > > > 
>> > > > NAK, no additional overhead in the software fast-path of
>> > > > sch_handle_{ingress,egress}() like this. There are users out there
>> > > > that use tc in software only, so performance is critical here.
>> > > 
>> > > Okay, how else do you suggest I can avoid the need to use tp->q?
>> > > I was thinking about storing q directly to net_device, which would safe
>> > > one dereference, resulting in the same amount as current cl->q.
>> > 
>> > Sorry for late reply, mostly off for few days. netdev struct has different
>> > cachelines which are hot on rx and tx (see also the location of the two
>> > lists, ingress_cl_list and egress_cl_list), if you add only one qdisc
>> > pointer there, then you'd at least penalize one of the two w/ potential
>> > cache miss. Can we leave it in cl?
>> 
>> Well that is the whole point of this excercise, to remove it from cl.
>> The thing is, for the shared blocks, cls are shared between multiple
>> qdisc instances, so cl->q makes no longer any sense.
>
>I don't really mind if you want to remove cl->q, but lets do it without
>adding any cost to the fast path. The primary motivation for this set
>is hw offload, but it shouldn't be at the expense to make sw fast path

That is not precise. The reason is not only hw offload. User could use
the block sharing for pure sw solution as well and benefit from that.


>slower. cl->q is only used here for updating stats, one possible option
>could be to take them out for the clsact case into dev at the expense
>for two pointers (e.g. getting rid of ingress_queue would reduce it to
>only one pointer), such that you have percpu {ingress,egress}_cl_stats
>that control path can fetch instead when dumping qdiscs. Could be one,
>but there are probably other options as well to explore.

I'm currently looking at that. Thanks.