> +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
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