From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 4CB1E46145 for ; Fri, 31 Jan 2025 00:06:57 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 282C34027C; Fri, 31 Jan 2025 00:06:57 +0100 (CET) Received: from mail-il1-f173.google.com (mail-il1-f173.google.com [209.85.166.173]) by mails.dpdk.org (Postfix) with ESMTP id 3526E4027C for ; Fri, 31 Jan 2025 00:06:56 +0100 (CET) Received: by mail-il1-f173.google.com with SMTP id e9e14a558f8ab-3ce915a8a25so5183495ab.1 for ; Thu, 30 Jan 2025 15:06:56 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind.com; s=google; t=1738278415; x=1738883215; darn=dpdk.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=8zLqPUFFZ0m3gxRqlfSBKr8QTLct2PwroMLnfwDKRv8=; b=isbeY4CK7PB7snC5gcsAumhhUl2BDAeEbtMfpahIZ0wl8zIFPUwxYlCJb0MnmdHTs3 MyReJL1GVorUnv1YnNS5MNJ65jHxrHj+2Z9lH+eiDx55cZptBli7ru/FDF9Kx7DLQkXY 6y0iky1lKORlii7P/7WpH0fHp0f/0ND0dylXM5W0YTcr4GL0UaKFtEecWYUwDkL9ZLbh TIhXouAO7tUOasHycpAXqlY/Ex5MObmiFovIGY+Kvlh8T1Cn0fnwyD/jxU7OogiLpmSf PJs018MA12uWPLJQkYDKHilIyM6K+s8zipNO7UFrSnQS827Zn3rWy1/T8oumju5jasJ6 UlnQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1738278415; x=1738883215; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=8zLqPUFFZ0m3gxRqlfSBKr8QTLct2PwroMLnfwDKRv8=; b=pve4cV4aCIWsDrQ+WPdQBVOmZ98gUOUGIG25+KfrNTSzaGG+Ua2WGlsvsz7ZJ6HBl9 zyuEzZUPbLowxV2/pNVh7D10vgPMXZp/dxW+RchY97l1766mq2FenvHsFJMWLuq2DXYI RNDMQKI212ekXQ8ZcfYmB45cZRQjoGmJw27Skkp62VgeYN4s+rNJC8/s7uw2dOSKKynh scHxWX4isKjUgZPbCYCcIfrauhzfjP7VUuyg6XMwyufgUZWFVp5Crvt5hb91Q7/7iBhM vmzhCal7iNkStid2rOtsOPGLqKB7fae0KH7EppC8/g8s5vylxS5eeAE3TPceBgXL4SYt I4fg== X-Forwarded-Encrypted: i=1; AJvYcCXGDsHQWzqyC2uPcrSZoZyNc+N694UWCNKXYQhOkAmh1m1A6SdrZttT5RqsrmdxuCEhDGgCEu4=@dpdk.org X-Gm-Message-State: AOJu0YxDp8aHl6urHnd9y5RlJX/VOupm2yXCJRbq68F7+hm1g1CcI1it +zw/CLcD+8OiBNGFOrxGCPnXpLEYe1ZWA6tQjrXNLontXXtrq4B/vPGP+nHyrwyVLzklLjSam3R Q4otfzV8XnBJEhz80i/M2o0qTVI2ZPeO8AyrQyw== X-Gm-Gg: ASbGncuYTPvziJRbxzyYbr8JG6WOw9A+eld8aJ2QdKjWfNdl20hAoY/NG7azCQwD9Lv 93GaLnBNdC2AFCjoEPGJC9Bcy7aKoKEVvRNSdAZkL2epn7SGLIR2jrVZOHFQRwk6w1yExsFCq9G ckxnbc3XkDtnFOp44OQJqoMixcApHt X-Google-Smtp-Source: AGHT+IH1ugkUZ1pmcLZ2lPw/sveWPu+I7qB2rlCVcg2YNdMjWJKWdLrWIebJGV5tl9JZUkTBdpxJpQwxZo7sMElzc6s= X-Received: by 2002:a05:6e02:12c8:b0:3ce:7d8f:3d75 with SMTP id e9e14a558f8ab-3cffe37c153mr82812245ab.1.1738278415378; Thu, 30 Jan 2025 15:06:55 -0800 (PST) MIME-Version: 1.0 References: <20250116195640.68885-1-ariel.otilibili@6wind.com> <20250128231152.249497-1-ariel.otilibili@6wind.com> <20250128231152.249497-3-ariel.otilibili@6wind.com> In-Reply-To: From: Ariel Otilibili Date: Fri, 31 Jan 2025 00:06:44 +0100 X-Gm-Features: AWEUYZm2AXBWXaCIaYEvmR6NzR5U6i3oEwb5H0RT6PFofbnWNx7xERnmRrInwIQ Message-ID: Subject: Re: [PATCH v3 2/2] net/af_xdp: Refactor af_xdp_tx_zc() To: Maryam Tahhan Cc: dev@dpdk.org, stable@dpdk.org, Thomas Monjalon , David Marchand , Stephen Hemminger , Ciara Loftus Content-Type: multipart/alternative; boundary="0000000000007847f0062cf47baf" X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: stable-bounces@dpdk.org --0000000000007847f0062cf47baf Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hello Maryam, On Wed, Jan 29, 2025 at 6:58=E2=80=AFPM Maryam Tahhan = 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 =3D 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 =3D 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 =3D 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.co= m/ > > Please let me know your thoughts, and I=E2=80=99d be happy to discuss fur= ther > > 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 --0000000000007847f0062cf47baf Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hello Maryam,

On= Wed, Jan 29, 2025 at 6:58=E2=80=AFPM 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)
> +{
> +=C2=A0 =C2=A0 =C2=A0struct rte_mbuf *ret =3D mbuf;
> +
> +=C2=A0 =C2=A0 =C2=A0if (!xsk_ring_prod__reserve(&txq->tx, 1, i= dx_tx)) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0kick_tx(txq, &txq= ->pair->cq);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (!xsk_ring_prod__r= eserve(&txq->tx, 1, idx_tx))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0ret =3D NULL;
> +=C2=A0 =C2=A0 =C2=A0}
> +
> +=C2=A0 =C2=A0 =C2=A0return ret;
> +}


[MT] I don't see why we are passing in mbuf here?
= =C2=A0My 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 w= ould create a fresh local_mbuf
> +static void
> +maybe_cpy_pkt(bool is_mbuf_equal, struct xsk_umem_info *umem,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0uint64_t addr_plus_offset, s= truct rte_mbuf *mbuf,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0struct xdp_desc *desc)
> +{
> +=C2=A0 =C2=A0 =C2=A0void *pkt;
> +
> +=C2=A0 =C2=A0 =C2=A0if(is_mbuf_equal)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0goto out;
> +
> +=C2=A0 =C2=A0 =C2=A0pkt =3D xsk_umem__get_data(umem->buffer, addr_= plus_offset);
> +=C2=A0 =C2=A0 =C2=A0rte_memcpy(pkt, rte_pktmbuf_mtod(mbuf, void *), d= esc->len);
> +=C2=A0 =C2=A0 =C2=A0rte_pktmbuf_free(mbuf);
> +
> +out:
> +=C2=A0 =C2=A0 =C2=A0return;
> +}


[MT] does this really need to be an inline function? it wasn't common <= br> code between the blocks?
In order for the next stateme= nts 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 u= mem; and the creation of local_mbuf accordingly
2. If local_m= buf is null, goto exit.
3. Else, update addr, offset, and descrip= tion
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...
Thank= s 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_t= x_zc(), but keep its core logic.

I wanted the boil= er plate to be into separate functions, so one could read through the subse= quent 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=E2=80=99d be happy to discuss furth= er

I improved on the snippets your proposal. It has fewe= r lines, so fewer changes.

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

Have a good day,
<= div>Ariel
--0000000000007847f0062cf47baf--