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: <f73daa0e-c201-5fba-8232-2107bc8e24de@intel.com>
Date:   Mon, 10 Jul 2023 15:13:22 +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:     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>, Eric Dumazet <edumazet@google.com>,
        Michal Kubiak <michal.kubiak@intel.com>,
        <intel-wired-lan@lists.osuosl.org>,
        Yunsheng Lin <linyunsheng@huawei.com>,
        David Christensen <drc@linux.vnet.ibm.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        "David S. Miller" <davem@davemloft.net>
References: <20230705155551.1317583-1-aleksander.lobakin@intel.com>
 <20230705155551.1317583-4-aleksander.lobakin@intel.com>
 <CAKgT0Ue+VvnzNUuKiO1XFW6w3Ka9=SSfGBP_KpkbvR6uzqvg5A@mail.gmail.com>
 <6310c483-8c6e-8d34-763a-487157f6ff0c@intel.com>
 <CAKgT0UfLBmzhshM5ZsvLaBwGtv2AvXA3n+kbn9FtBWTCocsiDw@mail.gmail.com>
From:   Alexander Lobakin <aleksander.lobakin@intel.com>
In-Reply-To: <CAKgT0UfLBmzhshM5ZsvLaBwGtv2AvXA3n+kbn9FtBWTCocsiDw@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?bkdTaFJFdVY3YnNCTVo0SFljcGpZcTdEVW1HTFl2cXg4NG4yMnpqQUxtNWR6?=
 =?utf-8?B?SmFRYnZtem41UjRKUmplTnRhTTJLaWhWWmR1Znd4WnBwSGdjNDRoRmtSTll2?=
 =?utf-8?B?ZWF4c0xMS0xKSDNOa3QwdmxJZGR3dVFUM3BJa25iQ3BTeFhHM25aR1FSU2ZY?=
 =?utf-8?B?NjZwemJQVFhSS2p1Nzh0NDNpL0hZUFFDWEJ3c2t0SlJJUUpKV004YXQrdHFr?=
 =?utf-8?B?a2ZlL0k1aHRYUkplVEZrUXN5WkpXREdzdVlCdnBnMnJXYjdSbFlyMWErcGl2?=
 =?utf-8?B?TmNNbjZwUUlrZFhuSkRDdnRGTnBKN2xKdGtabm9CWWo2ajFTSXlLVHA5elpH?=
 =?utf-8?B?cHVuZisxUWRzT1hPREZMYUlFeGJlM29Wdld4cDJJTVd0RW0wQkpCK0VFUmpP?=
 =?utf-8?B?SGJveVZDQVhTTXJoWUNMMnlnbTJYZno3b2lVcFJwSytTR1ZEU2IwOXZ3M0NK?=
 =?utf-8?B?Yk9kaTZ5VWRPSU5IZjJjcm1FMkdvTC9DS0lOZ0tyUDhoYnY5aVVXRlNWT2oz?=
 =?utf-8?B?amN1L3BibStTL0s1ZkpCWVJFKzUvbnVaVWtpdjRTbGE5M1B0dVJFcFhkQjRy?=
 =?utf-8?B?WCtFNTBpOE1UZWcra040czdveGNaV1dqOVl0SGdZWGJCbWhwTXA5eitvTEFH?=
 =?utf-8?B?Q2p5VFpYQ0hJcFMyVi9RTlZKd1g0ZkNnM0FkVzRkcUdLV01Nb0gxYkxENW5P?=
 =?utf-8?B?QUlzT1hnMmRQZisrci9Qd2g3ZVZoQXlWSUttM0hzT2NwaUVFRzJZWDUyUE9H?=
 =?utf-8?B?RGpZOXBTRi9QWFN2N05aTU5ybGFzb0I4OGtIRkhhSWNkVnBBbHdVVkFUMmJv?=
 =?utf-8?B?U1ZqaVhyTlFTNjhCc3ZJRzcyeDUyMi93L1lacnFlMXBGSFFMTjNSaGtXelNt?=
 =?utf-8?B?ZGU0QXA4NFF2aytTdjFMOWUvaTFIMjFMZjFxNXpzMFFzbkdoZUphWk0xaUV3?=
 =?utf-8?B?SzhZMitKWis3QStYb0pkdDM4a3h5QzN2bitsWkdQU0RHRUxnaFZSYzFadTU0?=
 =?utf-8?B?WitSdWVOOGgxRTVFVkF2WGpaN0dJdkdRaU5ncnNlbmFVTTRBSHU3aEg2UnA1?=
 =?utf-8?B?Mm15S1M4SFVWMkZDcEhOZ1h1R0hjZWxidXFVek1LTnlzb3BLcitkYzlkd1pX?=
 =?utf-8?B?N2dxQW40NVVaWFk2K01EY3JKaXJoU0xrTG1sWGpLeDdSY2Fjc041aU9nWUJn?=
 =?utf-8?B?U3lNa1FsRDNtQzkySG9ad0lVSzFITGMxa3N0VDA2ZWQ2UUNMTWhPRVZqM1hp?=
 =?utf-8?B?NVM2WDJ2Vzl0QlpIUkJWV1FIK055b2JOTDM3bEYvd3NOMUNiNm56Y2RtUHlr?=
 =?utf-8?B?MXZTU3dCUWkwN2lUMk8yV1FLYnVNbGNRc1VYL09oczJBaEhQN1B3UmdlRXhJ?=
 =?utf-8?B?MnhpMG9iSGsxUlluQTk4Tm5EelpHd0hqL29wV3YySkJ1YVRUUDRaNkRxNEZ0?=
 =?utf-8?B?NTJsNTdwT0d0Y3Jad3ZSNTgyOTNoOUxXVUNjbGZleXRCWDhkUWJHd1BWZkUy?=
 =?utf-8?B?Ly9Tbk9QaTdFVFlOKzBoNDZrajMyejZMUndyeVNLZC9nNkp6VWt6aU83ZUpR?=
 =?utf-8?B?czdUZTNmMHRDbzZGaDMwWmF4RkZESTk5ekdaRlZUeXg3V2R4Y3JnenUyMjJ1?=
 =?utf-8?B?UlFqcnZWSmZJNHIzK0Y2SmRlVVRVVXBoUmdiM0VoSVVmbU5kRlM3M1pUM2JE?=
 =?utf-8?B?ZzgwSzcxTU1CNkFIZ1c5N1ZYRmNTRDFVQ0tZaExwL0Y0RklGNG1hZGhqVjl2?=
 =?utf-8?B?dC9Ub0VpSkpkaTFOcDBxU1dIRkJmOVZIbjliWlRzV3NmWUs2TzJHSjBSVkFk?=
 =?utf-8?B?aVlQbVFpZmhjZTFLdjBqN3pDTUREaS81QjhOcFFuM3YvcXVLMTg5SWQrMWgr?=
 =?utf-8?B?T3ZQbklrQktwT3pRQmlRdXFuSFlwTkc3emhqNnlzbUk4b3g4dEF0TW5oQy85?=
 =?utf-8?B?Y1VaOStGVGlvVUpFQ1FFTUlYR2RNVlBIZTAyczRsWHVteEprejc0dEdCVDNW?=
 =?utf-8?B?TlhWbVdiV0NjSDBnaitZYUpKUkdQQ0lReG9SSnNxYzA4amE0VHJzTDczR1RB?=
 =?utf-8?B?cFBCVk56OHd6WURyZ2ZQdVBvWm45ZnpmVy8zbHJKYTVjQjFWQW5HWDBPeTMz?=
 =?utf-8?B?OUIzeXk2allrWDg0bVNoM3NEN0tFM0loRkl4czJtTlp3U29IVzY1WWw3MzNO?=
 =?utf-8?B?TEE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 8d9c8e87-bdfa-4744-e0f4-08db8147b853
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB3625.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Jul 2023 13:15:24.0776
 (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: l2MGT5aJuGPh2kcl1icQ7LADhaqWgHRVkm1FFwH20mmpx0IMlHGeGHD+gB4/l1Obl45GTwrLznC5oSiYqSklb4ja759vaIleHC3+lKlF3us=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR11MB6701
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.2 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_BLOCKED,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 10:06:29 -0700

> On Thu, Jul 6, 2023 at 9:46 AM Alexander Lobakin
> <aleksander.lobakin@intel.com> wrote:
>>
>> 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:
> 
> [...]
> 
>>>> @@ -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.
> 
> Specifically you don't want to be unmapping and freeing this page
> until after the !skb check. The problem is if skb is NULL the skb
> allocation failed and so processing of Rx is meant to stop in place
> without removing the page. It is where we will resume on the next pass
> assuming memory has been freed that can then be used. The problem is
> the skb allocation, not the page. We used to do the skb allocation
> before we would acquire the buffer, but with XDP there are cases where
> we aren't supposed to allocate it so it got moved to after which
> causes this confusion.
> 
>>>
>>>>                 /* 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.
> 
> The key bit here is the allocation failure is the reason why we halted
> processing. So the page contains valid data and should not be freed.
> We just need to leave it in place and wait for an allocation to
> succeed and then we can resume processing.

Aaah, okay, you want to try once again with the same buffer next time.
I see. Makes sense!

> 
>>>
>>>>
>>>> -               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.
> 
> Yeah, the iavf_put_rx_buffer should be left  down here.
Thanks,
Olek