Re: [PATCH net] net/mlx5e: Do not recycle pages from emergency reserve

[Saeed Mahameed <saeedm@dev.mellanox.co.il>]

On Sun, Jan 22, 2017 at 7:50 PM, Tom Herbert <tom@herbertland.com> wrote:
> On Sat, Jan 21, 2017 at 12:31 PM, Saeed Mahameed
> <saeedm@dev.mellanox.co.il> wrote:
>> On Sat, Jan 21, 2017 at 9:12 PM, kernel netdev <netdev@brouer.com> wrote:
>>>
>>>
>>> Den 21. jan. 2017 7.10 PM skrev "Tom Herbert" <tom@herbertland.com>:
>>>
>>> On Thu, Jan 19, 2017 at 11:14 AM, Saeed Mahameed
>>> <saeedm@dev.mellanox.co.il> wrote:
>>>> On Thu, Jan 19, 2017 at 9:03 AM, Eric Dumazet <eric.dumazet@gmail.com>
>>>> wrote:
>>>>> From: Eric Dumazet <edumazet@google.com>
>>>>>
>>>>> A driver using dev_alloc_page() must not reuse a page allocated from
>>>>> emergency memory reserve.
>>>>>
>>>>> Otherwise all packets using this page will be immediately dropped,
>>>>> unless for very specific sockets having SOCK_MEMALLOC bit set.
>>>>>
>>>>> This issue might be hard to debug, because only a fraction of received
>>>>> packets would be dropped.
>>>>
>>>> Hi Eric,
>>>>
>>>> When you say reuse, you mean point to the same page from several SKBs ?
>>>>
>>>> Because in our page cache implementation we don't reuse pages that
>>>> already passed to the stack,
>>>> we just keep them in the page cache until the ref count drop back to
>>>> one, so we recycle them (i,e they will be re-used only when no one
>>>> else is using them).
>>>>
>>> Saeed,
>>>
>>> Speaking of the mlx page cache can we remove this or a least make it
>>> optional to use. It is another example of complex functionality being
>>> put into drivers that makes things like backports more complicated and
>>
>> Re complexity, I am not sure the mlx page cache is that complex,
>> we just wrap alloc_page/put_page with our own page cache calls.
>> Roughly the page cache implementation is 200-300 LOC tops all concentrated
>> in one place in the code.
>>
> Taken as part of the RX buffer management code the whole thing in very
> complicated and seems to be completely bereft of any comments in the
> code as to how things are supposed to work.
>

The Idea is fairly simple, maybe we could have made it cleaner if we
better decoupled the page
cache from the rx logic.

The page cache works in parallel to the rx logic, it takes its owns ref counts,
this way the rx logic will continue working as if there was no cache
(get/put_pages).
but instead of of the kernel (get/put_pages) it will call the internal
cache get/put_cache_page.

Yeah I agree, some comments could help. We will work on this when we
fix the HOL issues.