Re: [PATCH net-next v2 2/6] tsnep: Add XDP TX support

On 09.12.22 03:23, Saeed Mahameed wrote:

quoted

+static int tsnep_xdp_tx_map(struct xdp_frame *xdpf, struct tsnep_tx *tx,
+                struct skb_shared_info *shinfo, int count,
+                bool dma_map)
+{
+    struct device *dmadev = tx->adapter->dmadev;
+    skb_frag_t *frag;
+    unsigned int len;
+    struct tsnep_tx_entry *entry;
+    void *data;
+    struct page *page;
+    dma_addr_t dma;
+    int map_len = 0;
+    int i;
+
+    frag = NULL;
+    len = xdpf->len;
+    for (i = 0; i < count; i++) {
+        entry = &tx->entry[(tx->write + i) % TSNEP_RING_SIZE];
+        if (dma_map) {

wouldn't it have made more sense if you passed TSNEP_TX_TYPE instead of
bool dma_map ?
here and in tsnep_xdp_xmit_frame_ring as well..

I will give it a try.

quoted

+/* This function requires __netif_tx_lock is held by the caller. */
+static int tsnep_xdp_xmit_frame_ring(struct xdp_frame *xdpf,
+                     struct tsnep_tx *tx, bool dma_map)
+{
+    struct skb_shared_info *shinfo = 
xdp_get_shared_info_from_frame(xdpf);
+    unsigned long flags;
+    int count = 1;
+    struct tsnep_tx_entry *entry;
+    int length;
+    int i;
+    int retval;

Maciiej already commented on this, and i agree with him, the whole series
needs some work on rev xmas tree variable declaration, code will look much
neater.

So far I ordered the variable declaration by variable usage with common
variables like i and retval at the end. I will take a look an that.

quoted

+
+    if (unlikely(xdp_frame_has_frags(xdpf)))
+        count += shinfo->nr_frags;
+
+    spin_lock_irqsave(&tx->lock, flags);
+
+    if (tsnep_tx_desc_available(tx) < (MAX_SKB_FRAGS + 1 + count)) {
+        /* prevent full TX ring due to XDP */
+        spin_unlock_irqrestore(&tx->lock, flags);
+
+        return -EBUSY;

You don't really do anything with the retval, so just return a boolean.

Will be changed.

quoted

+    }
+
+    entry = &tx->entry[tx->write];
+    entry->xdpf = xdpf;
+
+    retval = tsnep_xdp_tx_map(xdpf, tx, shinfo, count, dma_map);
+    if (retval < 0) {
+        tsnep_tx_unmap(tx, tx->write, count);
+        entry->xdpf = NULL;
+
+        tx->dropped++;
+
+        spin_unlock_irqrestore(&tx->lock, flags);
+
+        netdev_err(tx->adapter->netdev, "XDP TX DMA map failed\n");

please avoid printing in data path, find other means to expose such info.
stats
tracepoints
debug_message rate limited, etc ..

I will improve that.


Gerhard