| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <6b04def5-a3aa-1f77-b29d-bea4845e2678@redhat.com>
Date: Tue, 18 Apr 2023 14:45:52 +0200
From: Jesper Dangaard Brouer <jbrouer@...hat.com>
To: "Song, Yoong Siang" <yoong.siang.song@...el.com>,
"bpf@...r.kernel.org" <bpf@...r.kernel.org>,
Stanislav Fomichev <sdf@...gle.com>,
Toke Høiland-Jørgensen
<toke@...hat.com>
Cc: brouer@...hat.com,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"martin.lau@...nel.org" <martin.lau@...nel.org>,
"ast@...nel.org" <ast@...nel.org>,
"daniel@...earbox.net" <daniel@...earbox.net>,
"Lobakin, Aleksander" <aleksander.lobakin@...el.com>,
"Zaremba, Larysa" <larysa.zaremba@...el.com>,
"xdp-hints@...-project.net" <xdp-hints@...-project.net>,
"intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
"pabeni@...hat.com" <pabeni@...hat.com>,
"Brandeburg, Jesse" <jesse.brandeburg@...el.com>,
"kuba@...nel.org" <kuba@...nel.org>,
"edumazet@...gle.com" <edumazet@...gle.com>,
"john.fastabend@...il.com" <john.fastabend@...il.com>,
"hawk@...nel.org" <hawk@...nel.org>,
"davem@...emloft.net" <davem@...emloft.net>
Subject: Re: [PATCH bpf-next V1 2/5] igc: add igc_xdp_buff wrapper for
xdp_buff in driver
On 18/04/2023 06.34, Song, Yoong Siang wrote:
> On Monday, April 17, 2023 10:57 PM, Jesper Dangaard Brouer <brouer@...hat.com> wrote:
>> Driver specific metadata data for XDP-hints kfuncs are propagated via tail
>> extending the struct xdp_buff with a locally scoped driver struct.
>>
>> Zero-Copy AF_XDP/XSK does similar tricks via struct xdp_buff_xsk. This
>> xdp_buff_xsk struct contains a CB area (24 bytes) that can be used for extending
>> the locally scoped driver into. The XSK_CHECK_PRIV_TYPE define catch size
>> violations build time.
>>
>
> Since the main purpose of this patch is to introduce igc_xdp_buff, and
> you have another two patches for timestamp and hash,
> thus, suggest to move timestamp and hash related code into respective patches.
>
>> Signed-off-by: Jesper Dangaard Brouer <brouer@...hat.com>
>> ---
>> drivers/net/ethernet/intel/igc/igc.h | 6 ++++++
>> drivers/net/ethernet/intel/igc/igc_main.c | 30 ++++++++++++++++++++++-------
>> 2 files changed, 29 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/igc/igc.h
>> b/drivers/net/ethernet/intel/igc/igc.h
>> index f7f9e217e7b4..c609a2e648f8 100644
>> --- a/drivers/net/ethernet/intel/igc/igc.h
>> +++ b/drivers/net/ethernet/intel/igc/igc.h
>> @@ -499,6 +499,12 @@ struct igc_rx_buffer {
>> };
>> };
>>
>> +/* context wrapper around xdp_buff to provide access to descriptor
>> +metadata */ struct igc_xdp_buff {
>> + struct xdp_buff xdp;
>> + union igc_adv_rx_desc *rx_desc;
>
> Move rx_desc to 4th patch (Rx hash patch)
>
Hmm, rx_desc is also needed by 3rd patch (Rx timestamp), so that would
break...
I can reorder patches, and have "Rx hash patch" come before "Rx
timestamp" patch.
>> +};
>> +
>> struct igc_q_vector {
>> struct igc_adapter *adapter; /* backlink */
>> void __iomem *itr_register;
>> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c
>> b/drivers/net/ethernet/intel/igc/igc_main.c
>> index bfa9768d447f..3a844cf5be3f 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_main.c
>> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
>> @@ -2236,6 +2236,8 @@ static bool igc_alloc_rx_buffers_zc(struct igc_ring
>> *ring, u16 count)
>> if (!count)
>> return ok;
>>
>> + XSK_CHECK_PRIV_TYPE(struct igc_xdp_buff);
>> +
>> desc = IGC_RX_DESC(ring, i);
>> bi = &ring->rx_buffer_info[i];
>> i -= ring->count;
>> @@ -2520,8 +2522,8 @@ static int igc_clean_rx_irq(struct igc_q_vector
>> *q_vector, const int budget)
>> union igc_adv_rx_desc *rx_desc;
>> struct igc_rx_buffer *rx_buffer;
>> unsigned int size, truesize;
>> + struct igc_xdp_buff ctx;
>> ktime_t timestamp = 0;
>> - struct xdp_buff xdp;
>> int pkt_offset = 0;
>> void *pktbuf;
>>
>> @@ -2555,13 +2557,14 @@ static int igc_clean_rx_irq(struct igc_q_vector
>> *q_vector, const int budget)
>> }
>>
>> if (!skb) {
>> - xdp_init_buff(&xdp, truesize, &rx_ring->xdp_rxq);
>> - xdp_prepare_buff(&xdp, pktbuf - igc_rx_offset(rx_ring),
>> + xdp_init_buff(&ctx.xdp, truesize, &rx_ring->xdp_rxq);
>> + xdp_prepare_buff(&ctx.xdp, pktbuf - igc_rx_offset(rx_ring),
>> igc_rx_offset(rx_ring) + pkt_offset,
>> size, true);
>> - xdp_buff_clear_frags_flag(&xdp);
>> + xdp_buff_clear_frags_flag(&ctx.xdp);
>> + ctx.rx_desc = rx_desc;
>
> Move rx_desc to 4th patch (Rx hash patch)
Again would break 3rd patch.
>
>>
>> - skb = igc_xdp_run_prog(adapter, &xdp);
>> + skb = igc_xdp_run_prog(adapter, &ctx.xdp);
>> }
>>
>> if (IS_ERR(skb)) {
>> @@ -2583,9 +2586,9 @@ static int igc_clean_rx_irq(struct igc_q_vector
>> *q_vector, const int budget)
>> } else if (skb)
>> igc_add_rx_frag(rx_ring, rx_buffer, skb, size);
>> else if (ring_uses_build_skb(rx_ring))
>> - skb = igc_build_skb(rx_ring, rx_buffer, &xdp);
>> + skb = igc_build_skb(rx_ring, rx_buffer, &ctx.xdp);
>> else
>> - skb = igc_construct_skb(rx_ring, rx_buffer, &xdp,
>> + skb = igc_construct_skb(rx_ring, rx_buffer, &ctx.xdp,
>> timestamp);
>>
>> /* exit if we failed to retrieve a buffer */ @@ -2686,6 +2689,15
>> @@ static void igc_dispatch_skb_zc(struct igc_q_vector *q_vector,
>> napi_gro_receive(&q_vector->napi, skb); }
>>
>> +static struct igc_xdp_buff *xsk_buff_to_igc_ctx(struct xdp_buff *xdp) {
>> + /* xdp_buff pointer used by ZC code path is alloc as xdp_buff_xsk. The
>> + * igc_xdp_buff shares its layout with xdp_buff_xsk and private
>> + * igc_xdp_buff fields fall into xdp_buff_xsk->cb
>> + */
>> + return (struct igc_xdp_buff *)xdp; }
>> +
>
> Move xsk_buff_to_igc_ctx to 3th patch (timestamp patch), which is first patch
> adding xdp_metadata_ops support to igc.
>
Hmm, maybe, but that make the "wrapper" patch incomplete and then it
gets "completed" in the first patch that adds a xdp_metadata_ops.
>> static int igc_clean_rx_irq_zc(struct igc_q_vector *q_vector, const int budget) {
>> struct igc_adapter *adapter = q_vector->adapter; @@ -2704,6 +2716,7
>> @@ static int igc_clean_rx_irq_zc(struct igc_q_vector *q_vector, const int
>> budget)
>> while (likely(total_packets < budget)) {
>> union igc_adv_rx_desc *desc;
>> struct igc_rx_buffer *bi;
>> + struct igc_xdp_buff *ctx;
>> ktime_t timestamp = 0;
>> unsigned int size;
>> int res;
>> @@ -2721,6 +2734,9 @@ static int igc_clean_rx_irq_zc(struct igc_q_vector
>> *q_vector, const int budget)
>>
>> bi = &ring->rx_buffer_info[ntc];
>>
>> + ctx = xsk_buff_to_igc_ctx(bi->xdp);
>
> Move xsk_buff_to_igc_ctx to 3th patch (timestamp patch), which is first patch
> adding xdp_metadata_ops support to igc.
>
Sure, but it feels wrong to no "complete" the wrapper work in the
wrapper patch.
>> + ctx->rx_desc = desc;
>
> Move rx_desc to 4th patch (Rx hash patch)
>
I'll reorder patch 3 and 4, else it doesn't make any sense to gradually
introduce the members in wrapper struct igc_xdp_buff.
--Jesper
Powered by blists - more mailing lists