Jesper Dangaard Brouer <brouer@xxxxxxxxxx> writes:
On Fri, 7 May 2021 16:28:30 +0800
Yunsheng Lin <linyunsheng@xxxxxxxxxx> wrote:
On 2021/5/7 15:06, Ilias Apalodimas wrote:
> On Fri, May 07, 2021 at 11:23:28AM +0800, Yunsheng Lin wrote:
>> On 2021/5/6 20:58, Ilias Apalodimas wrote:
>>>>>>
>>>>>
...
>
>
> I think both choices are sane. What I am trying to explain
> here, is
> regardless of what we choose now, we can change it in the
> future without
> affecting the API consumers at all. What will change
> internally is the way we
> lookup the page pool pointer we are trying to recycle.
It seems the below API need changing?
+static inline void skb_mark_for_recycle(struct sk_buff *skb,
struct page *page,
+ struct xdp_mem_info *mem)
I don't think we need to change this API, to support future
memory
models. Notice that xdp_mem_info have a 'type' member.
Hi,
Providing that we will (possibly as a future optimization) store
the pointer to the page pool in struct page instead of strcut
xdp_mem_info, passing
xdp_mem_info * instead of struct page_pool * would mean that for
every packet we'll need to call
xa = rhashtable_lookup(mem_id_ht, &mem->id,
mem_id_rht_params);
xa->page_pool;
which might pressure the Dcache to fetch a pointer that might be
present already in cache as part of driver's data-structures.
I tend to agree with Yunsheng that it makes more sense to adjust
the API for the clear use-case now rather than using xdp_mem_info
indirection. It seems to me like
the page signature provides the same information anyway and allows
to support different memory types.
Shay
Naming in Computer Science is a hard problem ;-). Something that
seems
to confuse a lot of people is the naming of the struct
"xdp_mem_info".
Maybe we should have named it "mem_info" instead or
"net_mem_info", as
it doesn't indicate that the device is running XDP.
I see XDP as the RX-layer before the network stack, that helps
drivers
to support different memory models, also for handling normal
packets
that doesn't get process by XDP, and the drivers doesn't even
need to
support XDP to use the "xdp_mem_info" type.
