Hi Shay, On Sun, May 09, 2021 at 08:11:35AM +0300, Shay Agroskin wrote: > > 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. 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 > > > > > 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. >
