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

[Gerhard Engleder <gerhard@engleder-embedded.com>]

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