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: <6310c483-8c6e-8d34-763a-487157f6ff0c@intel.com>
Date:   Thu, 6 Jul 2023 18:45:14 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.12.0
Subject: Re: [Intel-wired-lan] [PATCH RFC net-next v4 3/9] iavf: drop page
 splitting and recycling
Content-Language: en-US
To:     Alexander Duyck <alexander.duyck@gmail.com>
CC:     "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Paul Menzel <pmenzel@molgen.mpg.de>,
        "Jesper Dangaard Brouer" <hawk@kernel.org>,
        Larysa Zaremba <larysa.zaremba@intel.com>,
        <netdev@vger.kernel.org>, Alexander Duyck <alexanderduyck@fb.com>,
        "Ilias Apalodimas" <ilias.apalodimas@linaro.org>,
        <linux-kernel@vger.kernel.org>,
        Yunsheng Lin <linyunsheng@huawei.com>,
        Michal Kubiak <michal.kubiak@intel.com>,
        <intel-wired-lan@lists.osuosl.org>,
        "David Christensen" <drc@linux.vnet.ibm.com>
References: <20230705155551.1317583-1-aleksander.lobakin@intel.com>
 <20230705155551.1317583-4-aleksander.lobakin@intel.com>
 <CAKgT0Ue+VvnzNUuKiO1XFW6w3Ka9=SSfGBP_KpkbvR6uzqvg5A@mail.gmail.com>
From:   Alexander Lobakin <aleksander.lobakin@intel.com>
In-Reply-To: <CAKgT0Ue+VvnzNUuKiO1XFW6w3Ka9=SSfGBP_KpkbvR6uzqvg5A@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?OHFURVkvazFxN1RwRHo4RGFPelFqWXZJdGg1a1d3MXh1VmlGVEliRE5FeSt0?=
 =?utf-8?B?cS9WdUR1ZHliQlZnVFd6akNDeXpxMDUvclA3VW5iU3l1eU9mL2prTE9waUJz?=
 =?utf-8?B?T0RDOVp1QllmcGt2aW9iYktUd0h2N3ZvTlp2ZXRDMFd0a1ZaZUtFaExDOWxK?=
 =?utf-8?B?TEZCRzlodFBUUUdySk1tRkQ2SE9iV0VPVDZvbytURVhOV1p5R2NDQkRjTXhp?=
 =?utf-8?B?MTRHQTdKanNNVGZLUWxzVlFUVHhnQnZjYW41VEtlYVNYQllUMW8xaUVCclNC?=
 =?utf-8?B?WnI0VVRwT1FlcVoraFRxbnYvd3FaaVlXVzJsRE0xZkZXZ1pYdEEzbHFFeS9S?=
 =?utf-8?B?Tk54NHc3TkxXaEw3QkNOM211dmJ2ZlBZTm5nckRsYThoR092RWZvL0FEVytZ?=
 =?utf-8?B?WmZWMEpPUHZtS1ZDUWJxbTY3dW1NcitBSFpld1ZGNEp6b1pCSjlSclIrTDBt?=
 =?utf-8?B?UmRRR2l4WE42bUNZb0Q1WGhmaXdMeVRMOGpHa0l0ZXROZzlodXI5YTFCSzlv?=
 =?utf-8?B?TVpIR2UrTWZvWTM3VkloOGE0ZVcySG9sZS9UK1MzeWRXUGlVRHdkVW1IN3Ew?=
 =?utf-8?B?WmVsdGh1ZlZKUzd6Tm5NeW1BeXRaR213SkIxNDBMWFVBcXRoYnF2QjhlOGt6?=
 =?utf-8?B?anVYNDlZZFNsWmloWEh5NTNiRW5CYVpheVAvTjNCVkNWVGxkblBtdVJDaXJI?=
 =?utf-8?B?OVJ4Yi85dnBlSU9BVUVnUUZ1Q2IzclVJL1NEQWNoS1drQm9WRkVhMkVvc1hU?=
 =?utf-8?B?ZHBvU3l3TW5wRXhZRWFObUhQd1l2MkJWcWltY1ZhSzJJTW5meXJSckVhOEZJ?=
 =?utf-8?B?UzV0ekJvTXozTStCS2J3Zi91b05WOGN1MEc1OUVvN0U4eW5oTFhLZ2V2d0ZK?=
 =?utf-8?B?dmh4ZFk1UlVQRDVwczBKR3ZnQzJ1QmgxZzZ2U3BJVUs1cVpDR2F0Z3pkdzdH?=
 =?utf-8?B?b1JwZGxpNGJlOWhZV0Q3MDV1NklFRnlodXJFd2dWOWR2Um9XWWpXS21TZmtR?=
 =?utf-8?B?WEVzZnVqalpLekRnb28wamZVamhUZlU0ZFZRMkxoYkhWUjZMRkx4dEx1RnZI?=
 =?utf-8?B?UnVsUS9BZnRKUUh6bkNRSlNrem0rZkJ6ZGNVcEpjNExKOUp5RzNIQmwrWG92?=
 =?utf-8?B?MXdWOWhKdHdhcUVMZEtPdkRQc2hYQ21NbTF6SW04aTVwaTZaalJqRUg0T1dp?=
 =?utf-8?B?YndVeHU5NXJXUHNOTnpmTVNlNEUwbDZzSmJ4eEhzb0xrcDkrbnJ3ME1XWnBQ?=
 =?utf-8?B?MUlGcWNyc3NHSjFrMWtocmR0cWFJZGIrL3RHRTFiaDhUbnFzOWh2RDFPQzhZ?=
 =?utf-8?B?NGsxcXVKdjNXR3U0ZlY4NDh6Mnc3TGpFbmlxVjRxaml3MTNpeXlHUkE5cmhH?=
 =?utf-8?B?eTJZbFFwK1ZnYzY0T3pxajY5Z2g1dXRtRXpHWkFUMDRheG1xcGIvd3hJdm5E?=
 =?utf-8?B?emk2blhWODBRTFlIUXlKUTc0YmFMSHF6WGp6Y2xYU1prY1VYQnVPd2tKckxa?=
 =?utf-8?B?MnJvb0U0WHg0Q2xPRURGS29yYldxQmdWcjMxV0JiajI0T0hSZmhjbG1TZWxM?=
 =?utf-8?B?WTZ3b2FJb01hbHgyM1NxS2hNMlZQazJBeHduekh0Rm5BaU04QXB5bEd6Lzdi?=
 =?utf-8?B?YStINUNUWHBIU1ZPVnhvak5WSG85cy9LQXJVKzJ6KzM5Qm9WeU1OSG9FTi9Q?=
 =?utf-8?B?Sjh4QVdoSVFQYkFFUk5mc3pGWWtsaHFUR0prKzMvUUlFMzRZSkdQRENrM3JR?=
 =?utf-8?B?SEo5ZmpkNDdEQm56THZjSFF5bXRrVGgzN3ZoT3NmMFJ2Zy9SSFNoancxaS91?=
 =?utf-8?B?SW9LUi81ZW05bU5xUXh0ZWYxNlVLTjR1UFh2Z3V6S2dBYXloUXkzU0VIMmRw?=
 =?utf-8?B?QUpaU1hPeXpPeHBlN0g2MEtySDRhSXgwajBHMnF5Zk1jWWtuWTl3V0JCWlZ2?=
 =?utf-8?B?Vlc3Tk5lODF4bFR6SVN6MGt3QVM4VTRZUHBiTW5wUXA3UnpTc1VrTEVKS1Ni?=
 =?utf-8?B?RXErSGllZFU2c0xFcTE2UVNINm1qbTNwSkJvM2Zpb3ZtNU5UQUVRNlRKR1pa?=
 =?utf-8?B?Snk3cmV3cVYvOEd6QlFsbEc1WUJZWFBMOWwzQWdhdGExM1RQRHY0NXJ5T0wv?=
 =?utf-8?B?U0ZzTWJVOHBXYWdQTXljYTJtcHUrRFVKOVpTMnZyZTc4a2gxbVc1NnE2UGZH?=
 =?utf-8?B?WXc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: aeb823cc-90f6-49b0-05fa-08db7e409475
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB3625.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Jul 2023 16:46:44.0909
 (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: T85XFS/zc1T2fi1PIVHW1oiS047Bxilk3z/czMcr95RgwOc8HhD9VWZwvw3Wm0PKSDVjiu+ASR8ws+xALB6bja8KnuJ9/f0QfJtgvhXZ+N4=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR11MB7930
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.5 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,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE
        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: Alexander Duyck <alexander.duyck@gmail.com>
Date: Thu, 6 Jul 2023 07:47:03 -0700

> On Wed, Jul 5, 2023 at 8:57 AM Alexander Lobakin
> <aleksander.lobakin@intel.com> wrote:
>>
>> As an intermediate step, remove all page splitting/recyclig code. Just
> 
> Spelling issue: "recycling"

checkpatch w/codespell didn't catch this one =\

> 
>> always allocate a new page and don't touch its refcount, so that it gets
>> freed by the core stack later.
>> Same for the "in-place" recycling, i.e. when an unused buffer gets
>> assigned to a first needs-refilling descriptor. In some cases, this
>> was leading to moving up to 63 &iavf_rx_buf structures around the ring
>> on a per-field basis -- not something wanted on hotpath.
>> The change allows to greatly simplify certain parts of the code:

[...]

>> @@ -1317,21 +1200,10 @@ static void iavf_put_rx_buffer(struct iavf_ring *rx_ring,
>>         if (!rx_buffer)
>>                 return;
>>
>> -       if (iavf_can_reuse_rx_page(rx_buffer)) {
>> -               /* hand second half of page back to the ring */
>> -               iavf_reuse_rx_page(rx_ring, rx_buffer);
>> -               rx_ring->rx_stats.page_reuse_count++;
>> -       } else {
>> -               /* we are not reusing the buffer so unmap it */
>> -               dma_unmap_page_attrs(rx_ring->dev, rx_buffer->dma,
>> -                                    iavf_rx_pg_size(rx_ring),
>> -                                    DMA_FROM_DEVICE, IAVF_RX_DMA_ATTR);
>> -               __page_frag_cache_drain(rx_buffer->page,
>> -                                       rx_buffer->pagecnt_bias);
>> -       }
>> -
>> -       /* clear contents of buffer_info */
>> -       rx_buffer->page = NULL;
>> +       /* we are not reusing the buffer so unmap it */
>> +       dma_unmap_page_attrs(rx_ring->dev, rx_buffer->dma,
>> +                            iavf_rx_pg_size(rx_ring),
>> +                            DMA_FROM_DEVICE, IAVF_RX_DMA_ATTR);
> 
> Rather than reorder all this I would just do the dma_unmap_page_attrs
> and then leave the assignment of NULL to rx_buffer->page. It should
> make this a bit easier to clean up the code below.
> 
>>  }
>>
>>  /**
>> @@ -1431,15 +1303,18 @@ static int iavf_clean_rx_irq(struct iavf_ring *rx_ring, int budget)
>>                 else
>>                         skb = iavf_build_skb(rx_ring, rx_buffer, size);
>>
>> +               iavf_put_rx_buffer(rx_ring, rx_buffer);
>> +
> 
> This should stay below where it was.

Wait-wait-wait.

if (!skb) break breaks the loop. put_rx_buffer() unmaps the page.
So in order to do the first, you need to do the second to avoid leaks.
Or you meant "why unmapping and freeing if we fail, just leave it in
place"? To make it easier to switch to Page Pool.

> 
>>                 /* exit if we failed to retrieve a buffer */
>>                 if (!skb) {
>>                         rx_ring->rx_stats.alloc_buff_failed++;
>> -                       if (rx_buffer && size)
>> -                               rx_buffer->pagecnt_bias++;
>> +                       __free_pages(rx_buffer->page,
>> +                                    iavf_rx_pg_order(rx_ring));
>> +                       rx_buffer->page = NULL;
>>                         break;
>>                 }
> 
> This code was undoing the iavf_get_rx_buffer decrement of pagecnt_bias
> and then bailing since we have halted forward progress due to an skb
> allocation failure. As such we should just be removing the if
> statement and the increment of pagecnt_bias.
> 
>>
>> -               iavf_put_rx_buffer(rx_ring, rx_buffer);
>> +               rx_buffer->page = NULL;
>>                 cleaned_count++;
>>
>>                 if (iavf_is_non_eop(rx_ring, rx_desc, skb))
> 
> If iavf_put_rx_buffer just does the unmap and assignment of NULL then
> it could just be left here as is.

I guess those two are tied with the one above.

[...]

Thanks,
Olek