| 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: <21f21dfb-264b-4e01-9cb3-8d0133b5b31b@ti.com>
Date: Mon, 3 Mar 2025 17:36:41 +0530
From: "Malladi, Meghana" <m-malladi@...com>
To: Dan Carpenter <dan.carpenter@...aro.org>
CC: <rogerq@...nel.org>, <danishanwar@...com>, <pabeni@...hat.com>,
<kuba@...nel.org>, <edumazet@...gle.com>, <davem@...emloft.net>,
<andrew+netdev@...n.ch>, <bpf@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
<netdev@...r.kernel.org>, <u.kleine-koenig@...libre.com>,
<matthias.schiffer@...tq-group.com>, <schnelle@...ux.ibm.com>,
<diogo.ivo@...mens.com>, <glaroque@...libre.com>, <macro@...am.me.uk>,
<john.fastabend@...il.com>, <hawk@...nel.org>, <daniel@...earbox.net>,
<ast@...nel.org>, <srk@...com>, Vignesh Raghavendra <vigneshr@...com>
Subject: Re: [EXTERNAL] Re: [PATCH net-next v3 3/3] net: ti: icssg-prueth: Add
XDP support
On 2/26/2025 4:19 PM, Dan Carpenter wrote:
> On Mon, Feb 24, 2025 at 04: 31: 02PM +0530, Meghana Malladi wrote: > @@
> -541,7 +558,153 @@ void emac_rx_timestamp(struct prueth_emac *emac, >
> ssh->hwtstamp = ns_to_ktime(ns); > } > > -static int
> emac_rx_packet(struct prueth_emac
> ZjQcmQRYFpfptBannerStart
> This message was sent from outside of Texas Instruments.
> Do not click links or open attachments unless you recognize the source
> of this email and know the content is safe.
> Report Suspicious
> <https://us-phishalarm-ewt.proofpoint.com/EWT/v1/G3vK!
> uldgnXdPPE27auXFPdnpH8jx2nnu2fsVXLMVOyEVUH9IX54g6v7RJRENIKzAm7XCuLfioMeFBSH4bAdUdQTaEArV63odoRERqTN_5Pk$>
> ZjQcmQRYFpfptBannerEnd
>
> On Mon, Feb 24, 2025 at 04:31:02PM +0530, Meghana Malladi wrote:
>> @@ -541,7 +558,153 @@ void emac_rx_timestamp(struct prueth_emac *emac,
>> ssh->hwtstamp = ns_to_ktime(ns);
>> }
>>
>> -static int emac_rx_packet(struct prueth_emac *emac, u32 flow_id)
>> +/**
>> + * emac_xmit_xdp_frame - transmits an XDP frame
>> + * @emac: emac device
>> + * @xdpf: data to transmit
>> + * @page: page from page pool if already DMA mapped
>> + * @q_idx: queue id
>> + *
>> + * Return: XDP state
>> + */
>> +int emac_xmit_xdp_frame(struct prueth_emac *emac,
>> + struct xdp_frame *xdpf,
>> + struct page *page,
>> + unsigned int q_idx)
>> +{
>> + struct cppi5_host_desc_t *first_desc;
>> + struct net_device *ndev = emac->ndev;
>> + struct prueth_tx_chn *tx_chn;
>> + dma_addr_t desc_dma, buf_dma;
>> + struct prueth_swdata *swdata;
>> + u32 *epib;
>> + int ret;
>> +
>> + void *data = xdpf->data;
>> + u32 pkt_len = xdpf->len;
>
> Get rid of these variables?
>
Yeah ok
>> +
>> + if (q_idx >= PRUETH_MAX_TX_QUEUES) {
>> + netdev_err(ndev, "xdp tx: invalid q_id %d\n", q_idx);
>> + return ICSSG_XDP_CONSUMED; /* drop */
>> + }
>> +
>> + tx_chn = &emac->tx_chns[q_idx];
>> +
>> + first_desc = k3_cppi_desc_pool_alloc(tx_chn->desc_pool);
>> + if (!first_desc) {
>> + netdev_dbg(ndev, "xdp tx: failed to allocate descriptor\n");
>> + goto drop_free_descs; /* drop */
>> + }
>> +
>> + if (page) { /* already DMA mapped by page_pool */
>> + buf_dma = page_pool_get_dma_addr(page);
>> + buf_dma += xdpf->headroom + sizeof(struct xdp_frame);
>> + } else { /* Map the linear buffer */
>> + buf_dma = dma_map_single(tx_chn->dma_dev, data, pkt_len, DMA_TO_DEVICE);
>> + if (dma_mapping_error(tx_chn->dma_dev, buf_dma)) {
>> + netdev_err(ndev, "xdp tx: failed to map data buffer\n");
>> + goto drop_free_descs; /* drop */
>> + }
>> + }
>> +
>> + cppi5_hdesc_init(first_desc, CPPI5_INFO0_HDESC_EPIB_PRESENT,
>> + PRUETH_NAV_PS_DATA_SIZE);
>> + cppi5_hdesc_set_pkttype(first_desc, 0);
>> + epib = first_desc->epib;
>> + epib[0] = 0;
>> + epib[1] = 0;
>> +
>> + /* set dst tag to indicate internal qid at the firmware which is at
>> + * bit8..bit15. bit0..bit7 indicates port num for directed
>> + * packets in case of switch mode operation
>> + */
>> + cppi5_desc_set_tags_ids(&first_desc->hdr, 0, (emac->port_id | (q_idx << 8)));
>> + k3_udma_glue_tx_dma_to_cppi5_addr(tx_chn->tx_chn, &buf_dma);
>> + cppi5_hdesc_attach_buf(first_desc, buf_dma, pkt_len, buf_dma, pkt_len);
>> + swdata = cppi5_hdesc_get_swdata(first_desc);
>> + if (page) {
>> + swdata->type = PRUETH_SWDATA_PAGE;
>> + swdata->data.page = page;
>> + } else {
>> + swdata->type = PRUETH_SWDATA_XDPF;
>> + swdata->data.xdpf = xdpf;
>> + }
>> +
>> + cppi5_hdesc_set_pktlen(first_desc, pkt_len);
>> + desc_dma = k3_cppi_desc_pool_virt2dma(tx_chn->desc_pool, first_desc);
>> +
>> + ret = k3_udma_glue_push_tx_chn(tx_chn->tx_chn, first_desc, desc_dma);
>> + if (ret) {
>> + netdev_err(ndev, "xdp tx: push failed: %d\n", ret);
>> + goto drop_free_descs;
>> + }
>> +
>> + return ICSSG_XDP_TX;
>> +
>> +drop_free_descs:
>> + prueth_xmit_free(tx_chn, first_desc);
>> + return ICSSG_XDP_CONSUMED;
>> +}
>> +EXPORT_SYMBOL_GPL(emac_xmit_xdp_frame);
>> +
>> +/**
>> + * emac_run_xdp - run an XDP program
>> + * @emac: emac device
>> + * @xdp: XDP buffer containing the frame
>> + * @page: page with RX data if already DMA mapped
>> + *
>> + * Return: XDP state
>> + */
>> +static int emac_run_xdp(struct prueth_emac *emac, struct xdp_buff *xdp,
>> + struct page *page)
>> +{
>> + struct net_device *ndev = emac->ndev;
>> + int err, result = ICSSG_XDP_PASS;
>> + struct bpf_prog *xdp_prog;
>> + struct xdp_frame *xdpf;
>> + int q_idx;
>> + u32 act;
>> +
>> + xdp_prog = READ_ONCE(emac->xdp_prog);
>> + act = bpf_prog_run_xdp(xdp_prog, xdp);
>> + switch (act) {
>> + case XDP_PASS:
>> + break;
>> + case XDP_TX:
>> + /* Send packet to TX ring for immediate transmission */
>> + xdpf = xdp_convert_buff_to_frame(xdp);
>> + if (unlikely(!xdpf))
>
> This is the second unlikely() macro which is added in this patchset.
> The rule with likely/unlikely() is that it should only be added if it
> likely makes a difference in benchmarking. Quite often the compiler
> is able to predict that valid pointers are more likely than NULL
> pointers so often these types of annotations don't make any difference
> at all to the compiled code. But it depends on the compiler and the -O2
> options.
>
Do correct me if I am wrong, but from my understanding, XDP feature
depends alot of performance and benchmarking and having unlikely does
make a difference. Atleast in all the other drivers I see this being
used for XDP.
>> + goto drop;
>> +
>> + q_idx = smp_processor_id() % emac->tx_ch_num;
>> + result = emac_xmit_xdp_frame(emac, xdpf, page, q_idx);
>> + if (result == ICSSG_XDP_CONSUMED)
>> + goto drop;
>> + break;
>> + case XDP_REDIRECT:
>> + err = xdp_do_redirect(emac->ndev, xdp, xdp_prog);
>> + if (err)
>> + goto drop;
>> + result = ICSSG_XDP_REDIR;
>> + break;
>> + default:
>> + bpf_warn_invalid_xdp_action(emac->ndev, xdp_prog, act);
>> + fallthrough;
>> + case XDP_ABORTED:
>> +drop:
>> + trace_xdp_exception(emac->ndev, xdp_prog, act);
>> + fallthrough; /* handle aborts by dropping packet */
>> + case XDP_DROP:
>> + ndev->stats.tx_dropped++;
>> + result = ICSSG_XDP_CONSUMED;
>> + page_pool_recycle_direct(emac->rx_chns.pg_pool, page);
>> + break;
>> + }
>> +
>> + return result;
>> +}
>> +
>> +static int emac_rx_packet(struct prueth_emac *emac, u32 flow_id, int *xdp_state)
>
>
> xdp_state should be a u32 because it's a bitfield. Bitfields are never
> signed.
Ok I will update it.
>
>> {
>> struct prueth_rx_chn *rx_chn = &emac->rx_chns;
>> u32 buf_dma_len, pkt_len, port_id = 0;
>> @@ -552,10 +715,12 @@ static int emac_rx_packet(struct prueth_emac *emac, u32 flow_id)
>> struct page *page, *new_page;
>> struct page_pool *pool;
>> struct sk_buff *skb;
>> + struct xdp_buff xdp;
>> u32 *psdata;
>> void *pa;
>> int ret;
>>
>> + *xdp_state = 0;
>> pool = rx_chn->pg_pool;
>> ret = k3_udma_glue_pop_rx_chn(rx_chn->rx_chn, flow_id, &desc_dma);
>> if (ret) {
>> @@ -596,9 +761,21 @@ static int emac_rx_packet(struct prueth_emac *emac, u32 flow_id)
>> goto requeue;
>> }
>>
>> - /* prepare skb and send to n/w stack */
>> pa = page_address(page);
>> - skb = napi_build_skb(pa, PAGE_SIZE);
>> + if (emac->xdp_prog) {
>> + xdp_init_buff(&xdp, PAGE_SIZE, &rx_chn->xdp_rxq);
>> + xdp_prepare_buff(&xdp, pa, PRUETH_HEADROOM, pkt_len, false);
>> +
>> + *xdp_state = emac_run_xdp(emac, &xdp, page);
>> + if (*xdp_state == ICSSG_XDP_PASS)
>> + skb = xdp_build_skb_from_buff(&xdp);
>> + else
>> + goto requeue;
>> + } else {
>> + /* prepare skb and send to n/w stack */
>> + skb = napi_build_skb(pa, PAGE_SIZE);
>> + }
>> +
>> if (!skb) {
>> ndev->stats.rx_dropped++;
>> page_pool_recycle_direct(pool, page);
>> @@ -861,13 +1038,23 @@ static void prueth_tx_cleanup(void *data, dma_addr_t desc_dma)
>> struct prueth_tx_chn *tx_chn = data;
>> struct cppi5_host_desc_t *desc_tx;
>> struct prueth_swdata *swdata;
>> + struct xdp_frame *xdpf;
>> struct sk_buff *skb;
>>
>> desc_tx = k3_cppi_desc_pool_dma2virt(tx_chn->desc_pool, desc_dma);
>> swdata = cppi5_hdesc_get_swdata(desc_tx);
>> - if (swdata->type == PRUETH_SWDATA_SKB) {
>> +
>> + switch (swdata->type) {
>> + case PRUETH_SWDATA_SKB:
>> skb = swdata->data.skb;
>> dev_kfree_skb_any(skb);
>> + break;
>> + case PRUETH_SWDATA_XDPF:
>> + xdpf = swdata->data.xdpf;
>> + xdp_return_frame(xdpf);
>> + break;
>> + default:
>> + break;
>> }
>>
>> prueth_xmit_free(tx_chn, desc_tx);
>> @@ -904,15 +1091,18 @@ int icssg_napi_rx_poll(struct napi_struct *napi_rx, int budget)
>> PRUETH_RX_FLOW_DATA_SR1 : PRUETH_RX_FLOW_DATA;
>> int flow = emac->is_sr1 ?
>> PRUETH_MAX_RX_FLOWS_SR1 : PRUETH_MAX_RX_FLOWS;
>> + int xdp_state_or = 0;
>> int num_rx = 0;
>> int cur_budget;
>> + int xdp_state;
>
> Both xdp_state_or and xdp_state should be u32 because they are bitfields.
>
Ok I will update it.
> regards,
> dan carpenter
>
>> int ret;
>>
>> while (flow--) {
>> cur_budget = budget - num_rx;
>>
>> while (cur_budget--) {
>> - ret = emac_rx_packet(emac, flow);
>> + ret = emac_rx_packet(emac, flow, &xdp_state);
>> + xdp_state_or |= xdp_state;
>> if (ret)
>> break;
>> num_rx++;
>
Powered by blists - more mailing lists