Re: [PATCH net-next v3 0/5] page_pool: recycle buffers

[Shay Agroskin]

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.