From mboxrd@z Thu Jan  1 00:00:00 1970
From: John Fastabend <john.fastabend@gmail.com>
Subject: Re: [RFC PATCH 06/17] net: sched: explicit locking in gso_cpu
 fallback
Date: Thu, 16 Nov 2017 05:31:20 -0800
Message-ID: <5c8e3ea0-8e6f-7431-96d6-f4fe94a02488@gmail.com>
References: <20171113195256.6245.64676.stgit@john-Precision-Tower-5810>
 <20171113200935.6245.70791.stgit@john-Precision-Tower-5810>
 <CAF=yD-+RS1v5vYy5ynQLaioc-527YN4a-P_+fvJ5AxeWLdqmwA@mail.gmail.com>
 <09820d68-ea84-5c9a-bb16-bc99f268c253@gmail.com>
 <CAF=yD-LD286JNvfS3LWfFSbd11-6eXDwYEDXjXkjjJbbyDntwA@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Cc: Daniel Borkmann <daniel@iogearbox.net>,
        Eric Dumazet <eric.dumazet@gmail.com>,
        Michael Ma <make0818@gmail.com>,
        Network Development <netdev@vger.kernel.org>,
        =?UTF-8?B?SmnFmcOtIFDDrXJr?= =?UTF-8?Q?o?= <jiri@resnulli.us>,
        Cong Wang <xiyou.wangcong@gmail.com>
To: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Return-path: <netdev-owner@vger.kernel.org>
Received: from mail-pf0-f195.google.com ([209.85.192.195]:57045 "EHLO
        mail-pf0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1751449AbdKPNbs (ORCPT
        <rfc822;netdev@vger.kernel.org>); Thu, 16 Nov 2017 08:31:48 -0500
Received: by mail-pf0-f195.google.com with SMTP id q4so11494355pfg.13
        for <netdev@vger.kernel.org>; Thu, 16 Nov 2017 05:31:47 -0800 (PST)
In-Reply-To: <CAF=yD-LD286JNvfS3LWfFSbd11-6eXDwYEDXjXkjjJbbyDntwA@mail.gmail.com>
Content-Language: en-US
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

On 11/15/2017 09:51 AM, Willem de Bruijn wrote:
> On Wed, Nov 15, 2017 at 10:11 AM, John Fastabend
> <john.fastabend@gmail.com> wrote:
>> On 11/14/2017 04:41 PM, Willem de Bruijn wrote:
>>>>  /* use instead of qdisc->dequeue() for all qdiscs queried with ->peek() */
>>>>  static inline struct sk_buff *qdisc_dequeue_peeked(struct Qdisc *sch)
>>>>  {
>>>> -       struct sk_buff *skb = sch->gso_skb;
>>>> +       struct sk_buff *skb = skb_peek(&sch->gso_skb);
>>>>
>>>>         if (skb) {
>>>> -               sch->gso_skb = NULL;
>>>> +               skb = __skb_dequeue(&sch->gso_skb);
>>>>                 qdisc_qstats_backlog_dec(sch, skb);
>>>>                 sch->q.qlen--;
>>>
>>> In lockless qdiscs, can this race, so that __skb_dequeue returns NULL?
>>> Same for its use in qdisc_peek_dequeued.
>>>
>>
>> Yes, agree if this was used in lockless qdisc it could race. However,
>> I don't think it is actually used in the lockless cases yet. For pfifo
>> fast __skb_array_peek is used.
> 
> Oh right. That will be easy to miss when other qdiscs are converted
> to lockless. Perhaps another location to add lockdep annotations.
> 

Yep. Will add lockdep here.

> Related: what happens when pfifo_fast is used as a leaf in a non-lockless
> qdisc hierarchy, say htb? The individual leaves will still have
> TCQ_F_NOLOCK, so will try to take the qdisc lock in dequeue_skb
> and other locations, but that is already held?
> 

Right. So I guess TCQ_F_NOLOCK needs to be propagated through the chain
of qdiscs and at attach we can only set TCQ_F_NOLOCK if the entire chain
of qdisc's are NOLOCK. Will spin an update with this as well.

> Thanks for revising and resubmitting the patchset, btw!
> 

no problem will be good to finally get this out of my queue.