| 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: <0d4b78ab-603d-e39d-f804-4f5d2f8efab8@intel.com>
Date: Thu, 5 Jan 2023 14:01:14 +0100
From: Alexander Lobakin <alexandr.lobakin@...el.com>
To: Gerhard Engleder <gerhard@...leder-embedded.com>
CC: <davem@...emloft.net>, <kuba@...nel.org>, <edumazet@...gle.com>,
<pabeni@...hat.com>, <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v3 4/9] tsnep: Add XDP TX support
From: Gerhard Engleder <gerhard@...leder-embedded.com>
Date: Wed Jan 04 2023 20:41:27 GMT+0100
> Implement ndo_xdp_xmit() for XDP TX support. Support for fragmented XDP
> frames is included.
>
> Signed-off-by: Gerhard Engleder <gerhard@...leder-embedded.com>
> ---
> drivers/net/ethernet/engleder/tsnep.h | 12 +-
> drivers/net/ethernet/engleder/tsnep_main.c | 208 ++++++++++++++++++++-
> 2 files changed, 209 insertions(+), 11 deletions(-)
[...]
> diff --git a/drivers/net/ethernet/engleder/tsnep_main.c b/drivers/net/ethernet/engleder/tsnep_main.c
> index 56c8cae6251e..2c7252ded23a 100644
> --- a/drivers/net/ethernet/engleder/tsnep_main.c
> +++ b/drivers/net/ethernet/engleder/tsnep_main.c
> @@ -310,10 +310,11 @@ static void tsnep_tx_activate(struct tsnep_tx *tx, int index, int length,
> struct tsnep_tx_entry *entry = &tx->entry[index];
>
> entry->properties = 0;
> - if (entry->skb) {
> + if (entry->skb || entry->xdpf) {
> entry->properties = length & TSNEP_DESC_LENGTH_MASK;
> entry->properties |= TSNEP_DESC_INTERRUPT_FLAG;
> - if (skb_shinfo(entry->skb)->tx_flags & SKBTX_IN_PROGRESS)
> + if (entry->type == TSNEP_TX_TYPE_SKB &&
> + skb_shinfo(entry->skb)->tx_flags & SKBTX_IN_PROGRESS)
Please enclose bitops (& here) hanging around any logical ops (&& here
in their own set of braces ().
> entry->properties |= TSNEP_DESC_EXTENDED_WRITEBACK_FLAG;
>
> /* toggle user flag to prevent false acknowledge
[...]
> @@ -417,12 +420,13 @@ static int tsnep_tx_unmap(struct tsnep_tx *tx, int index, int count)
> entry = &tx->entry[(index + i) % TSNEP_RING_SIZE];
>
> if (entry->len) {
> - if (i == 0)
> + if (i == 0 && entry->type == TSNEP_TX_TYPE_SKB)
`if (!i && ...)`, I think even checkpatch warns or was warning about
preferring !a over `a == 0` at some point.
> dma_unmap_single(dmadev,
> dma_unmap_addr(entry, dma),
> dma_unmap_len(entry, len),
> DMA_TO_DEVICE);
> - else
> + else if (entry->type == TSNEP_TX_TYPE_SKB ||
> + entry->type == TSNEP_TX_TYPE_XDP_NDO)
> dma_unmap_page(dmadev,
> dma_unmap_addr(entry, dma),
> dma_unmap_len(entry, len),
> @@ -502,12 +506,134 @@ static netdev_tx_t tsnep_xmit_frame_ring(struct sk_buff *skb,
> return NETDEV_TX_OK;
> }
>
> +static int tsnep_xdp_tx_map(struct xdp_frame *xdpf, struct tsnep_tx *tx,
> + struct skb_shared_info *shinfo, int count,
I believe most of those pointers, if not all of them, can be const, you
only read data from them (probably except @tx).
> + enum tsnep_tx_type type)
> +{
> + struct device *dmadev = tx->adapter->dmadev;
> + struct tsnep_tx_entry *entry;
> + struct page *page;
> + skb_frag_t *frag;
This one as well (also please check for @page, can't say for sure).
> + unsigned int len;
> + int map_len = 0;
> + dma_addr_t dma;
> + void *data;
> + int i;
[...]
> + entry->len = len;
> + dma_unmap_addr_set(entry, dma, dma);
> +
> + entry->desc->tx = __cpu_to_le64(dma);
> +
> + map_len += len;
> +
> + if ((i + 1) < count) {
Those braces are redundant here.
> + frag = &shinfo->frags[i];
> + len = skb_frag_size(frag);
> + }
> + }
> +
> + return map_len;
> +}
> +
> +/* This function requires __netif_tx_lock is held by the caller. */
> +static bool tsnep_xdp_xmit_frame_ring(struct xdp_frame *xdpf,
> + struct tsnep_tx *tx,
> + enum tsnep_tx_type type)
> +{
> + struct skb_shared_info *shinfo = xdp_get_shared_info_from_frame(xdpf);
Same for this one (const).
> + struct tsnep_tx_entry *entry;
> + int count = 1;
> + int length;
> + int retval;
> + int i;
Maybe squash some of them into one line as they are of the same type?
> +
> + if (unlikely(xdp_frame_has_frags(xdpf)))
> + count += shinfo->nr_frags;
> +
> + spin_lock_bh(&tx->lock);
[...]
> + retval = tsnep_xdp_tx_map(xdpf, tx, shinfo, count, type);
> + if (retval < 0) {
> + tsnep_tx_unmap(tx, tx->write, count);
> + entry->xdpf = NULL;
> +
> + tx->dropped++;
> +
> + spin_unlock_bh(&tx->lock);
> +
> + return false;
> + }
> + length = retval;
> +
> + for (i = 0; i < count; i++)
> + tsnep_tx_activate(tx, (tx->write + i) % TSNEP_RING_SIZE, length,
> + i == (count - 1));
Redundant braces around `count - 1`.
> + tx->write = (tx->write + count) % TSNEP_RING_SIZE;
> +
> + /* descriptor properties shall be valid before hardware is notified */
> + dma_wmb();
> +
> + spin_unlock_bh(&tx->lock);
> +
> + return true;
> +}
> +
> +static void tsnep_xdp_xmit_flush(struct tsnep_tx *tx)
> +{
> + iowrite32(TSNEP_CONTROL_TX_ENABLE, tx->addr + TSNEP_CONTROL);
> +}
> +
> static bool tsnep_tx_poll(struct tsnep_tx *tx, int napi_budget)
> {
> - int budget = 128;
> struct tsnep_tx_entry *entry;
> - int count;
> + struct xdp_frame_bulk bq;
> + int budget = 128;
BTW, why do you ignore NAPI budget and use your own?
> int length;
> + int count;
> +
> + xdp_frame_bulk_init(&bq);
> +
> + rcu_read_lock(); /* need for xdp_return_frame_bulk */
>
> spin_lock_bh(&tx->lock);
>
[...]
> @@ -552,8 +683,20 @@ static bool tsnep_tx_poll(struct tsnep_tx *tx, int napi_budget)
> skb_tstamp_tx(entry->skb, &hwtstamps);
> }
>
> - napi_consume_skb(entry->skb, budget);
> - entry->skb = NULL;
> + switch (entry->type) {
> + case TSNEP_TX_TYPE_SKB:
> + napi_consume_skb(entry->skb, budget);
> + entry->skb = NULL;
> + break;
> + case TSNEP_TX_TYPE_XDP_TX:
> + xdp_return_frame_rx_napi(entry->xdpf);
> + entry->xdpf = NULL;
> + break;
> + case TSNEP_TX_TYPE_XDP_NDO:
> + xdp_return_frame_bulk(entry->xdpf, &bq);
> + entry->xdpf = NULL;
> + break;
> + }
entry ::skb and ::xdpf share the same slot, you could nullify it here
once instead of duplicating the same op across each case.
>
> tx->read = (tx->read + count) % TSNEP_RING_SIZE;
>
> @@ -570,6 +713,10 @@ static bool tsnep_tx_poll(struct tsnep_tx *tx, int napi_budget)
>
> spin_unlock_bh(&tx->lock);
>
> + xdp_flush_frame_bulk(&bq);
> +
> + rcu_read_unlock();
> +
> return (budget != 0);
Also redundant braces.
> }
>
> @@ -1330,6 +1477,46 @@ static ktime_t tsnep_netdev_get_tstamp(struct net_device *netdev,
> return ns_to_ktime(timestamp);
> }
>
> +static int tsnep_netdev_xdp_xmit(struct net_device *dev, int n,
> + struct xdp_frame **xdp, u32 flags)
> +{
> + struct tsnep_adapter *adapter = netdev_priv(dev);
> + int cpu = smp_processor_id();
Can be u32.
> + struct netdev_queue *nq;
> + int nxmit;
> + int queue;
Squash?
> + bool xmit;
> +
> + if (unlikely(test_bit(__TSNEP_DOWN, &adapter->state)))
> + return -ENETDOWN;
> +
> + if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK))
> + return -EINVAL;
[...]
Thanks,
Olek
Powered by blists - more mailing lists