> -----Original Message-----
> From: Jason Wang <[email protected]>
> Sent: Wednesday, December 17, 2025 10:52 AM
> To: Angus Chen <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH] The vdpasim_net_work function handles both transmit TX
> completion and RX processing. A descriptor is taken from the TX VQ at the
> start
> of the loop, representing a buffer previously used for the TX path's initial
> read
> from the virtual networ...
>
> On Wed, Dec 17, 2025 at 10:30 AM Angus Chen <[email protected]>
> wrote:
> >
>
> It looks like the title is too long.
>
> > However, the error handling logic was flawed:
> >
> > 1. Read Failure (err <= 0): The TX VQ completion
> > (vdpasim_net_complete(txq, 0)) was performed, but the unconditional
> > break that followed prevented theprocessing of subsequent pending work
> > items (other descriptors) in the queue in the same work function call.
> >
> > 2. Write Failure (write <= 0): If data could not be written to the RX VQ
> > descriptor (e.g., vringh_iov_push_iotlb failed), the flow would 'break'
> > before the TX VQ descriptor was completed. This led to a leak of TX VQ
> > descriptors, as the work was never notified that the TX buffer was
> > free to use again.
> >
> > This commit refactors the control flow to ensure:
> > a) The TX VQ descriptor is completed back to the rx in all error and
> > success paths related to the current descriptor processing.
> > b) The unconditional break on read failure is removed, allowing the
> > function to continue processing remaining work items
> > until the loop naturally exits.
> >
> > Fixes: 0899774cb360 ("vdpa_sim_net: vendor satistics")
>
> This is suspicious, the logic was there since introduced? Or we can
> split this patch
Yes，you are right, I just use the 0899774cb360 which be influenced by the error
handler logic.
>
> 1) fix the stats
> 2) fix the error handling
>
> > ---
> >
> > Signed-off-by: Angus Chen <[email protected]>
> > ---
> > drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 28 ++++++++++++++--------------
> > 1 file changed, 14 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> > index 6caf09a1907b..298bd6e8e0a0 100644
> > --- a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> > +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> > @@ -242,22 +242,22 @@ static void vdpasim_net_work(struct vdpasim
> *vdpasim)
> > if (err <= 0) {
> > ++rx_overruns;
> > vdpasim_net_complete(txq, 0);
> > - break;
> > - }
> > -
> > - write = vringh_iov_push_iotlb(&rxq->vring, &rxq->in_iov,
> > - net->buffer, read);
> > - if (write <= 0) {
> > - ++rx_errors;
> > - break;
> > + } else {
>
> This else seem useless?
>
> > +
> > + write = vringh_iov_push_iotlb(&rxq->vring,
> &rxq->in_iov,
> > +
> net->buffer, read);
> > + if (write <= 0) {
> > + ++rx_errors;
> > + vdpasim_net_complete(txq, 0);
>
> I'd move this after:
>
> read = vringh_iov_pull_iotlb(&txq->vring, &txq->out_iov,
> net->buffer,
> PAGE_SIZE);
>
> So we would have a single vdpasim_net_complete()
Yes,I will send v2 to fix this.
>
> > + break;
> > + }
> > +
> > + ++rx_pkts;
> > + rx_bytes += write;
> > + vdpasim_net_complete(txq, 0);
> > + vdpasim_net_complete(rxq, write);
> > }
> >
> > - ++rx_pkts;
> > - rx_bytes += write;
> > -
> > - vdpasim_net_complete(txq, 0);
> > - vdpasim_net_complete(rxq, write);
> > -
> > if (tx_pkts > 4) {
> > vdpasim_schedule_work(vdpasim);
> > goto out;
> > --
> > 2.34.1
> >
>
> Thanks
