From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jesper Dangaard Brouer Subject: Re: Kernel 4.19 network performance - forwarding/routing normal users traffic Date: Mon, 5 Nov 2018 10:10:42 +0100 Message-ID: <20181105101042.269292ae@redhat.com> References: <61697e49-e839-befc-8330-fc00187c48ee@itcare.pl> <61e30474-b5e9-4dc8-a8a6-90cdd17d2a66@gmail.com> <8e10bf68-f3b3-98f2-91a5-25b151756dd6@itcare.pl> <20181101102213.2fa2643d@redhat.com> <20181101152716.GA13895@intel.com> <20181102052356.GA17587@intel.com> <20181102124037.352b15de@redhat.com> <20181102142024.GA18343@intel.com> <20181103135325.01a7b5d6@redhat.com> <20181105062836.GB4502@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: Saeed Mahameed , "pstaszewski@itcare.pl" , "eric.dumazet@gmail.com" , "netdev@vger.kernel.org" , Tariq Toukan , "ilias.apalodimas@linaro.org" , "yoel@kviknet.dk" , "mgorman@techsingularity.net" , brouer@redhat.com, =?UTF-8?B?SsOpcsO0bWU=?= Glisse To: Aaron Lu Return-path: Received: from mx1.redhat.com ([209.132.183.28]:39522 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726125AbeKES3n (ORCPT ); Mon, 5 Nov 2018 13:29:43 -0500 In-Reply-To: <20181105062836.GB4502@intel.com> Sender: netdev-owner@vger.kernel.org List-ID: On Mon, 5 Nov 2018 14:28:36 +0800 Aaron Lu wrote: > On Sat, Nov 03, 2018 at 01:53:25PM +0100, Jesper Dangaard Brouer wrote: > > > > On Fri, 2 Nov 2018 22:20:24 +0800 Aaron Lu wrote: > > > > > On Fri, Nov 02, 2018 at 12:40:37PM +0100, Jesper Dangaard Brouer wrote: > > > > On Fri, 2 Nov 2018 13:23:56 +0800 > > > > Aaron Lu wrote: > > > > > > > > > On Thu, Nov 01, 2018 at 08:23:19PM +0000, Saeed Mahameed wrote: > > > > > > On Thu, 2018-11-01 at 23:27 +0800, Aaron Lu wrote: > > > > > > > On Thu, Nov 01, 2018 at 10:22:13AM +0100, Jesper Dangaard Brouer > > > > > > > wrote: > > > > > > > ... ... > > > > > > > > Section copied out: > > > > > > > > > > > > > > > > mlx5e_poll_tx_cq > > > > > > > > | > > > > > > > > --16.34%--napi_consume_skb > > > > > > > > | > > > > > > > > |--12.65%--__free_pages_ok > > > > > > > > | | > > > > > > > > | --11.86%--free_one_page > > > > > > > > | | > > > > > > > > | |--10.10% > > > > > > > > --queued_spin_lock_slowpath > > > > > > > > | | > > > > > > > > | --0.65%--_raw_spin_lock > > > > > > > > > > > > > > This callchain looks like it is freeing higher order pages than order > > > > > > > 0: > > > > > > > __free_pages_ok is only called for pages whose order are bigger than > > > > > > > 0. > > > > > > > > > > > > mlx5 rx uses only order 0 pages, so i don't know where these high order > > > > > > tx SKBs are coming from.. > > > > > > > > > > Perhaps here: > > > > > __netdev_alloc_skb(), __napi_alloc_skb(), __netdev_alloc_frag() and > > > > > __napi_alloc_frag() will all call page_frag_alloc(), which will use > > > > > __page_frag_cache_refill() to get an order 3 page if possible, or fall > > > > > back to an order 0 page if order 3 page is not available. > > > > > > > > > > I'm not sure if your workload will use the above code path though. > > > > > > > > TL;DR: this is order-0 pages (code-walk trough proof below) > > > > > > > > To Aaron, the network stack *can* call __free_pages_ok() with order-0 > > > > pages, via: > > > > > > > > static void skb_free_head(struct sk_buff *skb) > > > > { > > > > unsigned char *head = skb->head; > > > > > > > > if (skb->head_frag) > > > > skb_free_frag(head); > > > > else > > > > kfree(head); > > > > } > > > > > > > > static inline void skb_free_frag(void *addr) > > > > { > > > > page_frag_free(addr); > > > > } > > > > > > > > /* > > > > * Frees a page fragment allocated out of either a compound or order 0 page. > > > > */ > > > > void page_frag_free(void *addr) > > > > { > > > > struct page *page = virt_to_head_page(addr); > > > > > > > > if (unlikely(put_page_testzero(page))) > > > > __free_pages_ok(page, compound_order(page)); > > > > } > > > > EXPORT_SYMBOL(page_frag_free); > > > > > > I think here is a problem - order 0 pages are freed directly to buddy, > > > bypassing per-cpu-pages. This might be the reason lock contention > > > appeared on free path. > > > > OMG - you just found a significant issue with the network stacks > > interaction with the page allocator! This explains why I could not get > > the PCP (Per-Cpu-Pages) system to have good performance, in my > > performance networking benchmarks. As we are basically only using the > > alloc side of PCP, and not the free side. > > Exactly. > > > We have spend years adding different driver level recycle tricks to > > avoid this code path getting activated, exactly because it is rather > > slow and problematic that we hit this zone->lock. > > I can see when this code path is hit, it causes unnecessary taking of > zone lock for order-0 pages and cause lock contention. > > > > > > Can someone apply below diff and see if lock contention is gone? > > > > I have also applied and tested this patch, and yes the lock contention > > is gone. As mentioned is it rather difficult to hit this code path, as > > the driver page recycle mechanism tries to hide/avoid it, but mlx5 + > > page_pool + CPU-map recycling have a known weakness that bypass the > > driver page recycle scheme (that I've not fixed yet). I observed a 7% > > speedup for this micro benchmark. > > Good to know this, I will prepare a formal patch. I wonder if this code is still missing something. I was looking at using put_devmap_managed_page() infrastructure, but I realized that page_frag_free() is also skipping this code path. I guess, I can add it later when I show/proof (performance wise) that this is a good idea (as we currently don't have any users). > > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > > > index e2ef1c17942f..65c0ae13215a 100644 > > > --- a/mm/page_alloc.c > > > +++ b/mm/page_alloc.c > > > @@ -4554,8 +4554,14 @@ void page_frag_free(void *addr) > > > { > > > struct page *page = virt_to_head_page(addr); > > > > > > - if (unlikely(put_page_testzero(page))) > > > - __free_pages_ok(page, compound_order(page)); > > > + if (unlikely(put_page_testzero(page))) { > > > + unsigned int order = compound_order(page); > > > + > > > + if (order == 0) > > > + free_unref_page(page); > > > + else > > > + __free_pages_ok(page, order); > > > + } > > > } > > > EXPORT_SYMBOL(page_frag_free); > > > > Thank you Aaron for spotting this!!! > > Which is impossible without your analysis :-) -- Best regards, Jesper Dangaard Brouer MSc.CS, Principal Kernel Engineer at Red Hat LinkedIn: http://www.linkedin.com/in/brouer