netdev - Re: [net-next PATCH] e1000: add initial XDP support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <57C3C944.2050902@gmail.com>
Date:   Sun, 28 Aug 2016 22:33:56 -0700
From:   John Fastabend <john.fastabend@...il.com>
To:     Or Gerlitz <gerlitz.or@...il.com>
Cc:     Brenden Blanco <bblanco@...mgrid.com>,
        David Miller <davem@...emloft.net>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        John Fastabend <john.r.fastabend@...el.com>,
        Linux Netdev List <netdev@...r.kernel.org>,
        Cong Wang <xiyou.wangcong@...il.com>
Subject: Re: [net-next PATCH] e1000: add initial XDP support

On 16-08-27 10:55 PM, Or Gerlitz wrote:
> On Sat, Aug 27, 2016 at 10:11 AM, John Fastabend
> <john.fastabend@...il.com> wrote:
>> From: Alexei Starovoitov <ast@...com>
> 
>> This patch adds initial support for XDP on e1000 driver. Note e1000
>> driver does not support page recycling in general which could be
>> added as a further improvement. However for XDP_DROP and XDP_XMIT
>> the xdp code paths will recycle pages.
> 
>> @@ -4188,15 +4305,57 @@ static bool e1000_clean_jumbo_rx_irq(struct e1000_adapter *adapter,
>>                 prefetch(next_rxd);
>>
>>                 next_buffer = &rx_ring->buffer_info[i];
>> -
> 
> nit, better to avoid random cleanups in a patch adding new (&& cool)
> functionality
> 

Yep thanks.

[...]

>> +                       case XDP_TX:
>> +                               dma_sync_single_for_device(&pdev->dev,
>> +                                                          dma,
>> +                                                          length,
>> +                                                          DMA_TO_DEVICE);
>> +                               e1000_xmit_raw_frame(buffer_info, length,
>> +                                                    netdev, adapter);
>> +                       /* Fallthrough to re-use mappedg page after xmit */
> 
> Did you want to say "mapped"? wasn't sure what's the role of "g" @ the end

Yep but see below...

> 
>> +                       case XDP_DROP:
>> +                       default:
>> +                               /* re-use mapped page. keep buffer_info->dma
>> +                                * as-is, so that e1000_alloc_jumbo_rx_buffers
>> +                                * only needs to put it back into rx ring
>> +                                */
> 
> if we're on the XDP_TX pass, don't we need to actually see that frame
> has been xmitted
> before re using the page?
> 

Agreed this seems to be too ambitious in the XDP_TX case. Thanks for
the help. Unless Alexei has some reason why it works I'll go ahead and
consume the buffer here.

I think setting

+                               bi->rxbuf.page = NULL;

at the end of the XDP_TX case should fix it but I'll test it again,

Thanks again I guess this is what I get for trying to push patches out
on Friday night.



>> +                               total_rx_bytes += length;
>> +                               total_rx_packets++;
>> +                               goto next_desc;
>> +                       }
>> +               }
>> +
>>                 dma_unmap_page(&pdev->dev, buffer_info->dma,
>>                                adapter->rx_buffer_len, DMA_FROM_DEVICE);
>>                 buffer_info->dma = 0;