From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id A8126C43381 for ; Thu, 21 Mar 2019 15:10:55 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 8414521874 for ; Thu, 21 Mar 2019 15:10:55 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728409AbfCUPKy (ORCPT ); Thu, 21 Mar 2019 11:10:54 -0400 Received: from mx1.redhat.com ([209.132.183.28]:57060 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728145AbfCUPKx (ORCPT ); Thu, 21 Mar 2019 11:10:53 -0400 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 878395F79F; Thu, 21 Mar 2019 15:10:53 +0000 (UTC) Received: from localhost.localdomain (unknown [10.32.181.117]) by smtp.corp.redhat.com (Postfix) with ESMTP id 2FDB25C21F; Thu, 21 Mar 2019 15:10:52 +0000 (UTC) Message-ID: <3530a9a2bf1eee8fff278945dc8a1dc122046940.camel@redhat.com> Subject: Re: [PATCH net-next 1/2] net: sched: add empty status flag for NOLOCK qdisc From: Paolo Abeni To: Eric Dumazet , netdev@vger.kernel.org Cc: "David S. Miller" , Eric Dumazet , John Fastabend , Ivan Vecera Date: Thu, 21 Mar 2019 16:10:51 +0100 In-Reply-To: References: Content-Type: text/plain; charset="UTF-8" User-Agent: Evolution 3.30.5 (3.30.5-1.fc29) MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Thu, 21 Mar 2019 15:10:53 +0000 (UTC) Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org Hi, Thank you for the review. On Thu, 2019-03-21 at 07:42 -0700, Eric Dumazet wrote: > > On 03/21/2019 03:14 AM, Paolo Abeni wrote: > > The queue is marked not empty after acquiring the seqlock, > > and it's up to the NOLOCK qdisc clearing such flag on dequeue. > > Since the empty status lays on the same cache-line of the > > seqlock, it's always hot on cache during the updates. > > > > This makes the empty flag update a little bit loosy. Given > > the lack of synchronization between enqueue and dequeue, this > > is unavoidable. > > > > Seems nice ! > > > Signed-off-by: Paolo Abeni > > --- > > include/net/sch_generic.h | 17 +++++++++++++++++ > > net/sched/sch_generic.c | 2 ++ > > 2 files changed, 19 insertions(+) > > > > diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h > > index 31284c078d06..1dc0aeec5d39 100644 > > --- a/include/net/sch_generic.h > > +++ b/include/net/sch_generic.h > > @@ -113,6 +113,9 @@ struct Qdisc { > > > > spinlock_t busylock ____cacheline_aligned_in_smp; > > spinlock_t seqlock; > > + > > + /* for NOLOCK qdisc, true if there are enqueued skbs */ > > + bool not_empty; > > struct rcu_head rcu; > > }; > > > > @@ -143,11 +146,25 @@ static inline bool qdisc_is_running(struct Qdisc *qdisc) > > return (raw_read_seqcount(&qdisc->running) & 1) ? true : false; > > } > > > > +static inline bool qdisc_is_empty(struct Qdisc *qdisc) > > +{ > > + if (qdisc->flags & TCQ_F_NOLOCK) > > + return !qdisc->not_empty; > > double negations are not very readable IMO ... > > Why not using qdisc->empty instead ? So that the initial kzalloc() initialize this field corrently and no explicit initialization is needed. Not a winning argument, I see. Will use 'empty' in next version. > > + return !qdisc->q.qlen; > > +} > > + > > +/* for NOLOCK qdisc, caller must held seqlock */ > > +static inline void qdisc_set_empty(struct Qdisc *qdisc) > > +{ > > + qdisc->not_empty = false; > > +} > > + > I guess the helper is not really needed ? I added this as an helper, to prepare the ground for other NOLOCK qdiscs. Since none of them is on the horizon, I'll drop this in the next version. I'd like to leverage this patch in a later series to avoid the qdisc_qstats_atomic_qlen_inc()/qdisc_qstats_atomic_qlen_dec() for NOLOCK qdisc, as you suggested in 46b1c18f9deb326a7e18348e668e4c7ab7c7458b In the control path we could resort to accumulate percpu values, and in the data path we always check for empty/not empty status, AFAICS with only one exception, in net/sched/sch_cbq.c: static inline void cbq_update_toplevel(struct cbq_sched_data *q, struct cbq_class *cl, struct cbq_class *borrowed) { if (cl && q->toplevel >= borrowed->level) { if (cl->q->q.qlen > 1) { This looks both problematic and strange to me. This test predates git history, and the related comment is not very helpful to me. Could it be a typo slipped in from the beginning ?!? (instead of 'cl->q->q.qlen > 0') Thanks! Paolo