Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210708164256.1.Ib344a977b52486ec81b60f9820338f1b43655f8d@changeid>
 <e13a1d38-cc0a-51b4-fe19-7a9145e75b1d@linux.intel.com>
In-Reply-To: <e13a1d38-cc0a-51b4-fe19-7a9145e75b1d@linux.intel.com>
From:   Ikjoon Jang <ikjn@chromium.org>
Date:   Mon, 19 Jul 2021 12:36:40 +0800
Message-ID: <CAATdQgDEwOZaBeXrsocF-b5-wfXgNGNhfaEBPsmhf7nefo55KQ@mail.gmail.com>
Subject: Re: [PATCH] xhci: fix unmatched num_trbs_free
To:     Mathias Nyman <mathias.nyman@linux.intel.com>
Cc:     linux-usb@vger.kernel.org,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Mathias Nyman <mathias.nyman@intel.com>,
        open list <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Mathias,

On Fri, Jul 16, 2021 at 8:54 PM Mathias Nyman
<mathias.nyman@linux.intel.com> wrote:
>
> On 8.7.2021 11.43, Ikjoon Jang wrote:
> > When unlinked urbs are queued to the cancelled td list, many tds
> > might be located after hw dequeue pointer and just marked as no-op
> > but not reclaimed to num_trbs_free. This bias can leads to unnecessary
> > ring expansions and leaks in atomic pool.
>
> Good point, in that case trbs turned no-op never get added to free trb count.
>
> >
> > To prevent this bias, this patch counts free TRBs every time xhci moves
> > dequeue pointer. This patch utilizes existing
> > update_ring_for_set_deq_completion() function, renamed it to move_deq().
> >
> > When it walks through to the new dequeue pointer, it also counts
> > free TRBs manually. This patch adds a fast path for the most cases
> > where the new dequeue pointer is still in the current segment.
> >
>
> This looks like an option.
>
> Another approach would be to keep the normal case fast, and the special case code simple.
> Something like:
>
> finish_td()
> ...
>         /* Update ring dequeue pointer */
>         if (ep_ring->dequeue == td->first_trb) {
>                 ep_ring->dequeue = td->last_trb;
>                 ep_ring->deq_seg = td->last_trb_seg;
>                 ep_ring->num_trbs_free += td->num_trbs - 1;
>                 inc_deq(xhci, ep_ring);
>         } else {
>                 move_deq(...);
>         }
>
> move_deq(...)
> {
>         while(ring->dequeue != new_dequeue)
>                 inc_deq(ring);
>         inc_deq(ring);
> }

Yes, I think most cases would be in (ep_ring->dequeue == td->first_trb)
so I think just repeating inc_deq() will be okay like the above example
cancelling urbs is an expensive and unusual operation.

But as you can see, I changed update_ring_for_set_deq_completion() to
move_deq(),
Do you think it's okay for that substitution In xhci_handle_cmd_set_deq()?
I'm worrying about some weird situation where the new dequeue ptr is
not in the ring.

>
> inc_deq() increases the num_trbs_free count.
>
> I haven't looked at the details of this yet, but I'm away for the next two weeks so
> I wanted to share this first anyway.
>

Thanks for reviewing, I hope to get some feedback when you come back.

> -Mathias

On Fri, Jul 16, 2021 at 8:54 PM Mathias Nyman
<mathias.nyman@linux.intel.com> wrote:
>
> On 8.7.2021 11.43, Ikjoon Jang wrote:
> > When unlinked urbs are queued to the cancelled td list, many tds
> > might be located after hw dequeue pointer and just marked as no-op
> > but not reclaimed to num_trbs_free. This bias can leads to unnecessary
> > ring expansions and leaks in atomic pool.
>
> Good point, in that case trbs turned no-op never get added to free trb count.
>
> >
> > To prevent this bias, this patch counts free TRBs every time xhci moves
> > dequeue pointer. This patch utilizes existing
> > update_ring_for_set_deq_completion() function, renamed it to move_deq().
> >
> > When it walks through to the new dequeue pointer, it also counts
> > free TRBs manually. This patch adds a fast path for the most cases
> > where the new dequeue pointer is still in the current segment.
> >
>
> This looks like an option.
>
> Another approach would be to keep the normal case fast, and the special case code simple.
> Something like:
>
> finish_td()
> ...
>         /* Update ring dequeue pointer */
>         if (ep_ring->dequeue == td->first_trb) {
>                 ep_ring->dequeue = td->last_trb;
>                 ep_ring->deq_seg = td->last_trb_seg;
>                 ep_ring->num_trbs_free += td->num_trbs - 1;
>                 inc_deq(xhci, ep_ring);
>         } else {
>                 move_deq(...);
>         }
>
> move_deq(...)
> {
>         while(ring->dequeue != new_dequeue)
>                 inc_deq(ring);
>         inc_deq(ring);
> }
>
> inc_deq() increases the num_trbs_free count.
>
> I haven't looked at the details of this yet, but I'm away for the next two weeks so
> I wanted to share this first anyway.
>
> -Mathias