Hi Mathias,
On Fri, Jul 16, 2021 at 8:54 PM Mathias Nyman
<mathias.nyman@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> 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
