netdev - Re: [PATCH net-next v1 08/11] xdp: tracking page

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: <20190618134847.GB5307@khorivan>
Date:   Tue, 18 Jun 2019 16:48:48 +0300
From:   Ivan Khoronzhuk <ivan.khoronzhuk@...aro.org>
To:     Ilias Apalodimas <ilias.apalodimas@...aro.org>
Cc:     Tariq Toukan <tariqt@...lanox.com>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Toke Høiland-Jørgensen <toke@...e.dk>,
        "toshiaki.makita1@...il.com" <toshiaki.makita1@...il.com>,
        "grygorii.strashko@...com" <grygorii.strashko@...com>,
        "mcroce@...hat.com" <mcroce@...hat.com>
Subject: Re: [PATCH net-next v1 08/11] xdp: tracking page_pool resources and
 safe removal

On Tue, Jun 18, 2019 at 04:30:12PM +0300, Ilias Apalodimas wrote:
>Hi Ivan, Tariq,
>
>> >>>+
>[...]
>> >>
>> >>What would you recommend to do for the following situation:
>> >>
>> >>Same receive queue is shared between 2 network devices. The receive ring is
>> >>filled by pages from page_pool, but you don't know the actual port (ndev)
>> >>filling this ring, because a device is recognized only after packet is
>> >>received.
>> >>
>> >>The API is so that xdp rxq is bind to network device, each frame has
>> >>reference
>> >>on it, so rxq ndev must be static. That means each netdev has it's own rxq
>> >>instance even no need in it. Thus, after your changes, page must be
>> >>returned to
>> >>the pool it was taken from, or released from old pool and recycled in
>> >>new one
>> >>somehow.
>> >>
>> >>And that is inconvenience at least. It's hard to move pages between
>> >>pools w/o
>> >>performance penalty. No way to use common pool either, as unreg_rxq now
>> >>drops
>> >>the pool and 2 rxqa can't reference same pool.
>> >>
>> >
>> >Within the single netdev, separate page_pool instances are anyway
>> >created for different RX rings, working under different NAPI's.
>>
>> The circumstances are so that same RX ring is shared between 2
>> netdevs... and netdev can be known only after descriptor/packet is
>> received. Thus, while filling RX ring, there is no actual device,
>> but when packet is received it has to be recycled to appropriate
>> net device pool. Before this change there were no difference from
>> which pool the page was allocated to fill RX ring, as there were no
>> owner. After this change there is owner - netdev page pool.
>>
>> For cpsw the dma unmap is common for both netdevs and no difference
>> who is freeing the page, but there is difference which pool it's
>> freed to.
>Since 2 netdevs are sharing one queue you'll need locking right?
>(Assuming that the rx-irq per device can end up on a different core)
No, rx-irq is not per device, same queue is shared only for descs,
farther it's separate queue with separate pool. Even rx-irq is per
device, no issues, as I said, it has it's own page pool, every queue
and every ndev, no need in locking, for pools...

>We discussed that ideally page pools should be alocated per hardware queue.
This is about one hw queue shared between ndevs. Page pool is separate
for each hw queues and each ndevs ofc.

>If you indeed need locking (and pay the performance penalty anyway) i wonder if
>there's anything preventing you from keeping the same principle, i.e allocate a
>pool per queue
page pool is per queue.

>pool per queue and handle the recycling to the proper ndev internally.
>That way only the first device will be responsible of
>allocating/recycling/maintaining the pool state.
No. There is more dependencies then it looks like, see rxq_info ...
The final recycling is ended not internally.

-- 
Regards,
Ivan Khoronzhuk