Re: [PATCH] xhci: fix unmatched num_trbs_free

[Ikjoon Jang <ikjn@chromium.org>]

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