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?
> +
> + 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.
> + 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.
> {
> 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.
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++;
