<div dir="ltr"><div dir="ltr">Hello Maryam,<br></div><br><div class="gmail_quote gmail_quote_container"><div dir="ltr" class="gmail_attr">On Wed, Jan 29, 2025 at 6:58 PM Maryam Tahhan <<a href="mailto:mtahhan@redhat.com">mtahhan@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +static struct rte_mbuf *<br>
> +maybe_kick_tx(struct pkt_tx_queue *txq, uint32_t *idx_tx, struct rte_mbuf *mbuf)<br>
> +{<br>
> + struct rte_mbuf *ret = mbuf;<br>
> +<br>
> + if (!xsk_ring_prod__reserve(&txq->tx, 1, idx_tx)) {<br>
> + kick_tx(txq, &txq->pair->cq);<br>
> + if (!xsk_ring_prod__reserve(&txq->tx, 1, idx_tx))<br>
> + ret = NULL;<br>
> + }<br>
> +<br>
> + return ret;<br>
> +}<br>
<br>
<br>
[MT] I don't see why we are passing in mbuf here?<br></blockquote><div> 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<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +static void<br>
> +maybe_cpy_pkt(bool is_mbuf_equal, struct xsk_umem_info *umem,<br>
> + uint64_t addr_plus_offset, struct rte_mbuf *mbuf,<br>
> + struct xdp_desc *desc)<br>
> +{<br>
> + void *pkt;<br>
> +<br>
> + if(is_mbuf_equal)<br>
> + goto out;<br>
> +<br>
> + pkt = xsk_umem__get_data(umem->buffer, addr_plus_offset);<br>
> + rte_memcpy(pkt, rte_pktmbuf_mtod(mbuf, void *), desc->len);<br>
> + rte_pktmbuf_free(mbuf);<br>
> +<br>
> +out:<br>
> + return;<br>
> +}<br>
<br>
<br>
[MT] does this really need to be an inline function? it wasn't common <br>
code between the blocks?<br></blockquote><div>In order for the next statements to just fall through till the exit. The loop would have read as such:</div><div><br></div><div>1. Some boiler plate to check if mbuf is equal to umem; and the creation of local_mbuf accordingly<br></div><div>2. If local_mbuf is null, goto exit.</div><div>3. Else, update addr, offset, and description</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Firstly thank you for your efforts to clean up the code. I believe a <br>
simpler cleanup approach would better balance readability + <br>
maintainability. This approach would focus on eliminating the repeated <br>
code in both branches of the conditional block. For me the main <br>
repetition between the branches is the code that reserves and fills the <br>
descriptor info, Please find an example below...<br></blockquote><div>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.</div><div><br></div><div>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.<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Note: The following is all untested code (don't even know if it <br>
compiles) it's just to give an idea around the cleanup I had in mind:<br></blockquote><div>The code did compile on the go, that was pretty neat. I cleared out few warnings, and pushed out a new version <br></div><div><a href="https://inbox.dpdk.org/dev/20250130221853.789366-3-ariel.otilibili@6wind.com/">https://inbox.dpdk.org/dev/20250130221853.789366-3-ariel.otilibili@6wind.com/</a></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Please let me know your thoughts, and I’d be happy to discuss further<br>
<br></blockquote><div>I improved on the snippets your proposal. It has fewer lines, so fewer changes.</div><div><br></div><div>What matters to me, is that the series be merged.</div><div><br></div><div>Have a good day,</div><div>Ariel<br></div></div></div>