Message-ID: <62b0a20232920_3573208ab@john.notmuch>
Date:   Mon, 20 Jun 2022 09:36:18 -0700
From:   John Fastabend <john.fastabend@...il.com>
To:     Maciej Fijalkowski <maciej.fijalkowski@...el.com>,
        John Fastabend <john.fastabend@...il.com>
Cc:     bpf@...r.kernel.org, ast@...nel.org, daniel@...earbox.net,
        netdev@...r.kernel.org, magnus.karlsson@...el.com,
        bjorn@...nel.org, kuba@...nel.org
Subject: Re: [PATCH v4 bpf-next 09/10] selftests: xsk: rely on pkts_in_flight
 in wait_for_tx_completion()

Maciej Fijalkowski wrote:
> On Fri, Jun 17, 2022 at 07:56:17PM -0700, John Fastabend wrote:
> > Maciej Fijalkowski wrote:
> > > Some of the drivers that implement support for AF_XDP Zero Copy (like
> > > ice) can have lazy approach for cleaning Tx descriptors. For ZC, when
> > > descriptor is cleaned, it is placed onto AF_XDP completion queue. This
> > > means that current implementation of wait_for_tx_completion() in
> > > xdpxceiver can get onto infinite loop, as some of the descriptors can
> > > never reach CQ.
> > > 
> > > This function can be changed to rely on pkts_in_flight instead.
> > > 
> > > Acked-by: Magnus Karlsson <magnus.karlsson@...el.com>
> > > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
> > > ---
> > 
> > Sorry I'm going to need more details to follow whats going on here.
> > 
> > In send_pkts() we do the expected thing and send all the pkts and
> > then call wait_for_tx_completion().
> > 
> > Wait for completion is obvious,
> > 
> >  static void wait_for_tx_completion(struct xsk_socket_info *xsk)               
> >  {                                                   
> >         while (xsk->outstanding_tx)                                                      
> >                 complete_pkts(xsk, BATCH_SIZE);
> >  }  
> > 
> > the 'outstanding_tx' counter appears to be decremented in complete_pkts().
> > This is done by looking at xdk_ring_cons__peek() makes sense to me until
> > it shows up here we don't know the pkt has been completely sent and
> > can release the resources.
> 
> This is necessary for scenarios like l2fwd in xdpsock where you would be
> taking entries from cq back to fq to refill the rx hw queue and keep going
> with the flow.
> 
> > 
> > Now if you just zero it on exit and call it good how do you know the
> > resources are safe to clean up? Or that you don't have a real bug
> > in the driver that isn't correctly releasing the resource.
> 
> xdpxceiver spawns two threads one for tx and one for rx. from rx thread
> POV if receive_pkts() ended its job then this implies that tx thread
> transmitted all of the frames that rx thread expected to receive. this
> zeroing is then only to terminate the tx thread and finish the current
> test case so that further cases under the current mode can be executed.
> 
> > 
> > How are users expected to use a lazy approach to tx descriptor cleaning
> > in this case e.g. on exit like in this case. It seems we need to
> > fix the root cause of ice not putting things on the completion queue
> > or I misunderstood the patch.
> 
> ice puts things on cq lazily on purpose as we added batching to Tx side
> where we clean descs only when it's needed.
> 
> We need to exit spawned threads before we detach socket from interface.
> Socket detach is done from main thread and in that case driver goes
> through tx ring and places descriptors that are left to completion queue.

But, in general (not this specific xdpxceiver) how does an application
that is tx only know when its OK to tear things down if the ice
driver can get stuck and never puts those on the completion queue? Should
there be some timer that fires and writes these back regardless of more
descriptors are seen? Did I understand the driver correctly.

Thanks,
John

Powered by blists - more mailing lists