Hi Shay,

On Sun, May 09, 2021 at 08:11:35AM +0300, Shay Agroskin wrote:

Jesper Dangaard Brouer [off-list ref] writes:

quoted

On Fri, 7 May 2021 16:28:30 +0800
Yunsheng Lin [off-list ref] wrote:

quoted

On 2021/5/7 15:06, Ilias Apalodimas wrote:

quoted

On Fri, May 07, 2021 at 11:23:28AM +0800, Yunsheng Lin wrote:  >>

On 2021/5/6 20:58, Ilias Apalodimas wrote:  >>>>>>  >>>>>

...

quoted

quoted

quoted

quoted

I think both choices are sane.  What I am trying to explain >

here, is

quoted

regardless of what we choose now, we can change it in the > future

without

quoted

affecting the API consumers at all.  What will change > internally

is the way we

quoted

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.

We've switched the patches already.  We didn't notice any performance boost
by doing so (tested on a machiattobin), but I agree as well.  As I
explained the only thing that will change if we ever the need the struct
xdp_mem_info in struct page is the internal contract between struct page
and the recycling function, so let's start clean and see if we ever need
that.


Cheers
/Ilias

Shay

quoted

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.