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

[Gerhard Engleder <gerhard@xxxxxxxxxxxxxxxxxxxxx>]

On 09.12.22 03:23, Saeed Mahameed wrote:
> > +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.

> > +/* 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.

> > +
> > +    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.

> > +    }
> > +
> > +    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