Re: [PATCH v3 2/2] net/af_xdp: Refactor af_xdp_tx_zc()

[Ariel Otilibili <ariel.otilibili@6wind.com>]

[-- Attachment #1: Type: text/plain, Size: 3053 bytes --]

Hello Maryam,

On Wed, Jan 29, 2025 at 6:58 PM Maryam Tahhan <mtahhan@redhat.com> wrote:

> > +static struct rte_mbuf *
> > +maybe_kick_tx(struct pkt_tx_queue *txq, uint32_t *idx_tx, struct
> rte_mbuf *mbuf)
> > +{
> > +     struct rte_mbuf *ret = mbuf;
> > +
> > +     if (!xsk_ring_prod__reserve(&txq->tx, 1, idx_tx)) {
> > +             kick_tx(txq, &txq->pair->cq);
> > +             if (!xsk_ring_prod__reserve(&txq->tx, 1, idx_tx))
> > +                     ret = NULL;
> > +     }
> > +
> > +     return ret;
> > +}
>
>
> [MT] I don't see why we are passing in mbuf here?
>
 My aim was to use the output of maybe_kick_tx() for the if statement on
local_buf: the true leg would copy mbuf into local_mbuf, and the false
would create a fresh local_mbuf

> > +static void
> > +maybe_cpy_pkt(bool is_mbuf_equal, struct xsk_umem_info *umem,
> > +           uint64_t addr_plus_offset, struct rte_mbuf *mbuf,
> > +           struct xdp_desc *desc)
> > +{
> > +     void *pkt;
> > +
> > +     if(is_mbuf_equal)
> > +             goto out;
> > +
> > +     pkt = xsk_umem__get_data(umem->buffer, addr_plus_offset);
> > +     rte_memcpy(pkt, rte_pktmbuf_mtod(mbuf, void *), desc->len);
> > +     rte_pktmbuf_free(mbuf);
> > +
> > +out:
> > +     return;
> > +}
>
>
> [MT] does this really need to be an inline function? it wasn't common
> code between the blocks?
>
In order for the next statements to just fall through till the exit. The
loop would have read as such:

1. Some boiler plate to check if mbuf is equal to umem; and the creation of
local_mbuf accordingly
2. If local_mbuf is null, goto exit.
3. Else, update addr, offset, and description

> Firstly thank you for your efforts to clean up the code. I believe a
> simpler cleanup approach would better balance readability +
> maintainability. This approach would focus on eliminating the repeated
> code in both branches of the conditional block. For me the main
> repetition between the branches is the code that reserves and fills the
> descriptor info, Please find an example below...
>
Thanks to you, I do appreciate your honest feedback. From what I
understand, you would like to take the filling of addr, offset, and desc
off from af_xdp_tx_zc(), but keep its core logic.

I wanted the boiler plate to be into separate functions, so one could read
through the subsequent statements. So we could avoid the cascade of if
statements.

>
> Note: The following is all untested code (don't even know if it
> compiles) it's just to give an idea around the cleanup I had in mind:
>
The code did compile on the go, that was pretty neat. I cleared out few
warnings, and pushed out a new version
https://inbox.dpdk.org/dev/20250130221853.789366-3-ariel.otilibili@6wind.com/

>
> Please let me know your thoughts, and I’d be happy to discuss further
>
> I improved on the snippets your proposal. It has fewer lines, so fewer
changes.

What matters to me, is that the series be merged.

Have a good day,
Ariel

[-- Attachment #2: Type: text/html, Size: 4486 bytes --]