Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <7ddd93fd-437d-8d67-c693-1bbcf36b552a@intel.com>
Date:   Wed, 31 May 2023 17:22:37 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH net-next v2 03/12] iavf: optimize Rx buffer allocation a
 bunch
Content-Language: en-US
To:     Maciej Fijalkowski <maciej.fijalkowski@intel.com>
CC:     Alexander H Duyck <alexander.duyck@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Magnus Karlsson <magnus.karlsson@intel.com>,
        Michal Kubiak <michal.kubiak@intel.com>,
        "Larysa Zaremba" <larysa.zaremba@intel.com>,
        Jesper Dangaard Brouer <hawk@kernel.org>,
        Ilias Apalodimas <ilias.apalodimas@linaro.org>,
        "Christoph Hellwig" <hch@lst.de>,
        Paul Menzel <pmenzel@molgen.mpg.de>, <netdev@vger.kernel.org>,
        <intel-wired-lan@lists.osuosl.org>, <linux-kernel@vger.kernel.org>
References: <20230525125746.553874-1-aleksander.lobakin@intel.com>
 <20230525125746.553874-4-aleksander.lobakin@intel.com>
 <8828262f1c238ab28be9ec87a7701acd791af926.camel@gmail.com>
 <ZHcsBPr0EXx6hkkd@boxer>
From:   Alexander Lobakin <aleksander.lobakin@intel.com>
In-Reply-To: <ZHcsBPr0EXx6hkkd@boxer>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?Wit2dm1wbkpzVDk4Nk52REk2M2NNQnFGbW9IZlFrN1RTdjArM3NNVC81V1pP?=
 =?utf-8?B?NGcrTEZ2MmRxdWF6dDFVOTN3VHFSSzQrU0hQMHNESENweVdqcFRNdTFtdnRU?=
 =?utf-8?B?TVpydHZyVll5T2gwUC9kdTluSWREZWdIVkplckhlY0J2cm9kSXV5NTFmd3ZT?=
 =?utf-8?B?WmE0RXZOaFdva3BPQ0lWR2xHdkJ6NnMxdSsvU21jcEM1YjQ1Q3hvbEx5cmhM?=
 =?utf-8?B?UVRHc0p2djB2ZjdBYUliNEZlUUZxb0dXcFh4TFFqdFpxNUJjbzhZOE9JNzc0?=
 =?utf-8?B?cXJ3aFJ2azdEYWVlN2kzcDNqYlFaOVdYT01lT3NaV0JhTGZrbmhXZ1VLQkFG?=
 =?utf-8?B?SWRQdXFTSVFNaFdRTXNBWHMxeGJlOUNEajJ4c2pWMzdxYzU1YmhDRTB2YlF5?=
 =?utf-8?B?OENEWHhUUFlqc2VTcnRTNU5WbnNGb2N1ajFUQ3RIckk4eTYrVEc3MEF1SlV2?=
 =?utf-8?B?R0RKemNQaWYwOGNjQmtlZEtsTmIwNEN3NGdwRXZKYml2Y1Nuems1WGRMZitN?=
 =?utf-8?B?TFk1Y1pWQjBDSlNjWmhyQ3RvUlp1WElwaHNzL2JreEJpcC9EUW9pb3gwaHFV?=
 =?utf-8?B?MTFoaXl1M0lJWUlCdHpVSDFSYndyTFN2OHc2aGs3QnFlUVMwQlpXWTBNNlo4?=
 =?utf-8?B?OFdBYXNqWDV2WU5ueUJ0WEttZHRlSHNoVHc0d3oxMnhiZzlIYytpNmxQM2E2?=
 =?utf-8?B?ckZIQ0lZc0lzMkZVOXBaQW5IWUlzZFA3TkZ6bEdrSTI1OUZocjNQb0RXTThl?=
 =?utf-8?B?WDVqK212R3JUNVBMRDlsZzU2N0lCOUlHMFRVdWJZTW10dEdOaURVRk5STUJK?=
 =?utf-8?B?TDJXNGVIS1l4MXNMN0Z4enB5VkZnREZkek8zTWpzRkF1WHVnQ04vVW5OT3pi?=
 =?utf-8?B?d3E0TktPQ3dGQ3dRbWZHeEpEUGpQRFdRSGV5cUJUbEFSNlZSMm1EZXpDcTlw?=
 =?utf-8?B?UG9KbHlMYUUxS005M0NVQUVmRE9zNEhYcENsRi9ZeTNMQWlYNkFpRDBWVkl1?=
 =?utf-8?B?akYxNjR4NnhlU1BYd1c0MHpBb3FyZnp0MytncEp0SldySUd4Vk1IRWpRbWtk?=
 =?utf-8?B?NjBUL1FtOW5mV0VQSjNJUFVPMWpFaFlEZGRsVmhkN0V2eFFTTHJWNG5hS0Rk?=
 =?utf-8?B?TDVac3NvU0NzaGhkVlhzcUF1endpbjRMQjBPYkdLbGd0U0ZoUHNWbk5GSXMz?=
 =?utf-8?B?L0lma0JKM3g1WjdOTjZOd2gwUXZxaFgyU2RkZlk4Wit3ZnFYMWY4eFhCMkZB?=
 =?utf-8?B?cE01blNDYkUwNitZaU1ZY25oSmJnM21YaWl3LzFnemxGb3FuRU1GTENTU24y?=
 =?utf-8?B?cTRwV1RaY3o3ZUgvOFVKKzlIZEVIcmEya25qVGlLbTQvWHJmdUg4cElKNEtR?=
 =?utf-8?B?bGRtQ09heEdCOFhxTVlVRE8zSXcxU2tmQ2tSTDhseUhYM05qQkE1eDdJWE04?=
 =?utf-8?B?ZXRiU3NBSjZZd2I1VGVwemxXRCtyUnBuNW1uSEpyS3FPRDI0VjlHRG4vUjZx?=
 =?utf-8?B?Nkx5Mk9sRm1sRkZKaUw5YmpMcW1lSHJJdTNxNDZBZ2ZSVWQxM2FLU0x3a2tK?=
 =?utf-8?B?K21Na0tMdEgweVBqcWdyay9VOFAzNjVPT2tTRVBKNkdINW84b1B5OE9pc3Na?=
 =?utf-8?B?THBuU201MmxpT2kxYkdDT3lpY1Y5MGhxSlBFejZvSW1jbTFvQzRnd0pQVldr?=
 =?utf-8?B?ZnNpdnJma1psd0RyN3BaeEljdGNCUU9uS25mWnlQOEFtWlpsNTAxcW9ETVhh?=
 =?utf-8?B?MjZUcDhHRXhPNlFMZW95cTQrQ0xNSCtOTnBmWjZSbDFKQisxZ1BudjVLdkdx?=
 =?utf-8?B?OGFhdDNYRHZkZjB3bTBxeFl4dldwbVdsSCtPdzZWU09GelR5UGdFTTZib2Rl?=
 =?utf-8?B?SnA5TUIvSGpyVVg3QWtvY0FXeW42Y0c5SHRTMTU1MzNxT1Z5YlBKeXVIY2pa?=
 =?utf-8?B?V2QwSzc5VHJqdm5tMWdWck53bW9qRUE4WUpLQ2tJOXRweTBEL0VLVFo3L3dn?=
 =?utf-8?B?d0hzM0JZa1dIL3hJdFBzRGFzT1JBbGJ3dTQwSjExM0RzaXlEa1ZzUm1UMGht?=
 =?utf-8?B?dHE4ZDRKNVVQNmxBWFNHREFtMmpDeU9sSDk4bE1lbkphMUM1VnVxRi8xeElz?=
 =?utf-8?B?Y1lXMGRiMDE1aktGSy9tNXB0ajVSTWVGWXN2VTVST3Qwd3hoaFlzT3RlMmw0?=
 =?utf-8?B?Znc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: ad4be76c-b0d9-4612-5422-08db61eb02f7
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB3625.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 May 2023 15:23:40.1331
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: GJQpfhsNGniQqRWQipfJncluAKuZkayvel0V8KqrjLREu1KPzGaReOz25Z3obV/Ywaqq2kwRECabHzcRHwE36mbYlFJO06HvLuk9FiMdwFM=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR11MB4677
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.7 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,
        SPF_NONE,T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Date: Wed, 31 May 2023 13:14:12 +0200

> On Tue, May 30, 2023 at 09:18:40AM -0700, Alexander H Duyck wrote:
> 
> FWIW I agree with what Alex is saying over here.

There are 2 Alexes, "choose wisely" :P

> 
>> On Thu, 2023-05-25 at 14:57 +0200, Alexander Lobakin wrote:
>>> The Rx hotpath code of IAVF is not well-optimized TBH. Before doing any
>>> further buffer model changes, shake it up a bit. Notably:
>>>
>>> 1. Cache more variables on the stack.
>>>    DMA device, Rx page size, NTC -- these are the most common things
>>>    used all throughout the hotpath, often in loops on each iteration.
>>>    Instead of fetching (or even calculating, as with the page size) them
>>>    from the ring all the time, cache them on the stack at the beginning
>>>    of the NAPI polling callback. NTC will be written back at the end,
>>>    the rest are used read-only, so no sync needed.
>>
>> The advantage of this is going to vary based on the attribute. One of
>> the reasons why I left most of this on the ring is because the section
>> of the ring most of these variables were meant to be read-mostly and
>> shouldn't have resulted in any additional overhead versus accessing
>> them from the stack.
> 
> I believe it depends on ring struct layout which vary across our drivers,
> no? On ice using making more usage of stack as described above improved
> perf.

It's +/- the same, most layout changes usually come with us moving stuff
around to optimize paddings and cachelines lol. Here's the same as with
ice, I don't think it's driver specific to get some positive results
from shortcutting more hotties. The sole time I was surprised is when
you were getting worse results storing xdp_buff on the stack vs on the
ring :D

> 
>>
>>> 2. Don't move the recycled buffers around the ring.
>>>    The idea of passing the page of the right-now-recycled-buffer to a
>>>    different buffer, in this case, the first one that needs to be
>>>    allocated, moreover, on each new frame, is fundamentally wrong. It
>>>    involves a few o' fetches, branches and then writes (and one Rx
>>>    buffer struct is at least 32 bytes) where they're completely unneeded,
>>>    but gives no good -- the result is the same as if we'd recycle it
>>>    inplace, at the same position where it was used. So drop this and let
>>>    the main refilling function take care of all the buffers, which were
>>>    processed and now need to be recycled/refilled.
>>
>> The next_to_alloc logic was put in place to deal with systems that are
>> experiencing memory issues. Specifically what can end up happening is
>> that the ring can stall due to failing memory allocations and the
>> memory can get stuck on the ring. For that reason we were essentially
>> defragmenting the buffers when we started suffering memory pressure so
>> that they could be reusued and/or freed following immediate use.
>>
>> Basically what you are trading off is some exception handling for
>> performance by removing it.
> 
> With all of the mix of the changes this patch carries, I find it hard to
> follow from description which parts of diff I should be looking at.

Huge piece removed before add_rx_frag_blah.

> 
>>
>>> 3. Don't allocate with %GPF_ATOMIC on ifup.
>>>    This involved introducing the @gfp parameter to a couple functions.
>>>    Doesn't change anything for Rx -> softirq.

[...]

>>> + up to 2% performance.
>>>
>>
>> What is the test you saw the 2% performance improvement in? Is it
>> something XDP related or a full stack test?
> 
> +1, can you say more about that measurement?

My prev reply to Alex.

> 
>>
>>> Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>
>>
>> Also one thing I am not a huge fan of is a patch that is really a
>> patchset onto itself. With all 6 items called out here I would have
>> preferred to see this as 6 patches as it would have been easier to
>> review.
> 
> +1

+1 :D

[...]

>>>  	/* if we are the last buffer then there is nothing else to do */
>>>  #define IAVF_RXD_EOF BIT(IAVF_RX_DESC_STATUS_EOF_SHIFT)
>>>  	if (likely(iavf_test_staterr(rx_desc, IAVF_RXD_EOF)))
>>
>> You may want to see if you can get rid of this function entirely,
>> perhaps you do in a later patch. This function was added for ixgbe back
>> in the day to allow us to place the skb back in the ring for the RSC
>> based workloads where we had to deal with interleaved frames in the Rx
>> path.
>>
>> For example, one question here would be why are we passing skb? It
>> isn't used as far as I can tell.
>>
> this was used back when skb was stored within the Rx buffer and now we
> just store skb on Rx ring struct, so good catch, this arg is redundant.

Also prev reply. I'm removing it later in the series hehe.

> 
> I'll go and take a look at code on v3.

No changes apart fixing OcteonTX2 compilation =\

Thanks,
Olek