Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
To:     Eric Dumazet <eric.dumazet@gmail.com>, <kerneljasonxing@gmail.com>,
        <anthony.l.nguyen@intel.com>, <davem@davemloft.net>,
        <kuba@kernel.org>, <ast@kernel.org>, <daniel@iogearbox.net>,
        <hawk@kernel.org>, <john.fastabend@gmail.com>, <andrii@kernel.org>,
        <kafai@fb.com>, <songliubraving@fb.com>, <yhs@fb.com>,
        <kpsingh@kernel.org>
CC:     <intel-wired-lan@lists.osuosl.org>, <netdev@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <bpf@vger.kernel.org>,
        Jason Xing <xingwanli@kuaishou.com>,
        Shujin Li <lishujin@kuaishou.com>
References: <20210826141623.8151-1-kerneljasonxing@gmail.com>
 <00890ff4-3264-337a-19cc-521a6434d1d0@gmail.com>
From:   Jesse Brandeburg <jesse.brandeburg@intel.com>
Subject: Re: [PATCH v4] ixgbe: let the xdpdrv work with more than 64 cpus
Message-ID: <860ead37-87f4-22fa-e4f3-5cadd0f2c85c@intel.com>
Date:   Thu, 26 Aug 2021 09:41:19 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.13.0
In-Reply-To: <00890ff4-3264-337a-19cc-521a6434d1d0@gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-MessageSentRepresentingType: 1
Received: from [192.168.1.214] (50.39.107.76) by MW4P220CA0012.NAMP220.PROD.OUTLOOK.COM (2603:10b6:303:115::17) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4457.17 via Frontend Transport; Thu, 26 Aug 2021 16:41:21 +0000
X-MS-PublicTrafficType: Email
X-MS-Office365-Filtering-Correlation-Id: 5ea1d46f-ede4-47f1-c5fe-08d968b055f5
X-MS-TrafficTypeDiagnostic: MWHPR11MB2000:
X-MS-Exchange-Transport-Forked: True
X-Microsoft-Antispam-PRVS: <MWHPR11MB2000B301749F655E50A5F2A697C79@MWHPR11MB2000.namprd11.prod.outlook.com>
X-MS-Oob-TLC-OOBClassifiers: OLM:8882;
X-MS-Exchange-SenderADCheck: 1
X-MS-Exchange-AntiSpam-Relay: 0
X-Microsoft-Antispam: BCL:0;
X-Microsoft-Antispam-Message-Info: 9oD6t3nSRhXw1kkyJe4kX+jXlrkJrW6vwbSfXckvAgL9ciEO8JwVX2PjMeM9sAdOPrImL8Wdr4F6hXkZF4mjCSvlbcp3HNWSnpnNyZOp2uz3NmdXNkOF0OlcH00WD6L0eiC+pi8jZE1dTZFol3qyNSuwXY/rzJC9IPks0ZQ8Yg2+KXAC/y2mzUaAQRXr8ubQoAbcnaRt8XmA2jws9TWC1DayE+nrdlzLfMmte/3dXKq4a7z1Tw3BZ345KUfUKis9citkHMhVzV7gugZCF5xEGmYjZTLhsXo1YaRvGcnB1IXb5j8LDWOfEoDNvAHAGJTxT0HexgMTSFGG6U3SoOC6nPQ+ftU+wk31oh1V8sVD2RKQvJfLQO0q1HmWD4teNhMhgS/Gbl+jsw0F2UmdgHBPTQjQgR3zTErHNtP61b+uA+uO/ondYO4Oj7qeuG+RZDf9ntRU3SB/ZGrg5Q3MZVJoUCiDTqde3n+7IM9VPEQ05oWlcNA7rqS/3pqcJQmN7Ry96KHq7n5RwgC0TuLbMbfUX/hFemYXsrrbpw7JVymWmzzoo/pAvxqcKhKDrWx3IsNqGJ8SYxyX06dHthiG8DBWoYrBiMOCIhSPQLS1QUKnaXinf16bRReLarIKNLTZ615HV0DUc5fVoMdxOAft3ahzhrwWlR77TWOjZLUtiOlv2hXtM5aAIxl0Hp5oOYZqu9JfzMDCnl6g1pP4OsUUp4x3/5v8BBNxAp3NvisEjJL+QukZG93D389gzk7S7xT3l7MZTSF9pEYUOfAIPUhe9v3viUiFZ78q0HLrwCa2FonqPqo=
X-Forefront-Antispam-Report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:CO1PR11MB4914.namprd11.prod.outlook.com;PTR:;CAT:NONE;SFS:(4636009)(366004)(53546011)(86362001)(36756003)(316002)(921005)(31696002)(54906003)(8676002)(38100700002)(186003)(8936002)(5660300002)(66556008)(2906002)(6486002)(66946007)(66476007)(2616005)(4326008)(956004)(7416002)(44832011)(508600001)(16576012)(83380400001)(31686004)(26005)(40753002)(45980500001)(43740500002);DIR:OUT;SFP:1102;
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?OEM3ZVB5TVhsNXg3Z2d4R0pJSXZNVkdHZzF0Nmk1U3F1U2JTbWlPZTJkVmww?=
 =?utf-8?B?bXBTSXJqcXNCVGZkQjYycFRmVnR3NXBxeE9ib09XOGVJSlB0ejloQ1NzMm1t?=
 =?utf-8?B?SUVPbXpwUiswaHJIb2o5dU1VKzFBZi94emJsM3Q4dkNIQXp1VnB0TWt2UUlM?=
 =?utf-8?B?aHg0Qm1GWG42N1FYSjhkL2MrSTdEV01zRUtJR2tCem1NR2Y4d2VwNGg0d1A5?=
 =?utf-8?B?eTBHTXBKK1BlY3U2a1Z2bHlkalg5c3NFLzdkc3pjSHZTV2FIY1FEZG51bkpX?=
 =?utf-8?B?MGFUS2E4UFArSEg0bWJLOUlCL0QzZ295S09JTDVMU1p3ZythTi9iUUkzNkox?=
 =?utf-8?B?N0kvd3N4Tm9KNFF4Mi8rNHNVRDJtdDlSTjZCUnk5WU5wSDlvQlFVWTZJWElz?=
 =?utf-8?B?cWtrdHRHc1RYQ3ZBcEw0dXVlMmZ2QmVsdDZuM2JnT2VINTcybVp3NGVtL0VS?=
 =?utf-8?B?U3VldkRQT0VTUVhWVnc0QVg4TDkza3BoeXg1SVFyc2xLdFo4UzBkUWdmOHgr?=
 =?utf-8?B?dVBHYmt2Zi94d1FlTC9ybDQxSTNtak5DNHpQaFBHN3hHSDFJK3pacmU0Q3cx?=
 =?utf-8?B?OUM4Y1kyM3BNNHh1TWNXTFBOUmx5WFZoQ1BITlkrMHNENHpiSGF6NmtsRlBh?=
 =?utf-8?B?ZXRlUTdvdzJsUnBhSERKWk9jbXVYRklIM0RqSVY2dkRJUi82M1pTS0RxMGx6?=
 =?utf-8?B?d2NjUHBnYkdTNmxMM1ZNa1h2QWJQSnFMamN0ZExGRm1USTlCUFZ3b09LUFkz?=
 =?utf-8?B?TDFTVVhGVjRCWGpqYnpGekNtTmoxUWNIcVNqNXUvWVBURGZwZTJVQzlGOFpM?=
 =?utf-8?B?NW9WQjU1WkVkL2VJZ1l2b1Z3TGFQQjhDTkk2V0JjYnNBVG5FQm1TN2FNc0l1?=
 =?utf-8?B?UFJXRDlqcWtkQjJJU1VGdHFCVVB2S1JIV2pHNXNaUlNUZUd0RTFRd1VzdW1M?=
 =?utf-8?B?dWZIalltNk1zMytzcUszb2szcCtOc0d3Z0l4RUZuSDExT0MzZFBhaFVnclBh?=
 =?utf-8?B?cVVPbkNGb0ZZNU1OeXZPNTBvMXhEWHNyakZyUTBmWTdVYVpyZkJXSmdwT3Jj?=
 =?utf-8?B?N0tJV0VCeHhRUTEyOTlsWHhtQk1Gd09vYTB5WUYxNjEyR0ZKcndsUlAvb1dV?=
 =?utf-8?B?VlZnN1BrTkdUWDRrYkJYM2dEeVBoRERvSDJkWktHbk9sS05oaFdOWnVoOCtI?=
 =?utf-8?B?UCtXVS9reGpTK2RwejJrTkl4VFdsNkZyQUhLeTA1bHBaZkxOQjYxeHZqdHpo?=
 =?utf-8?B?a3B4UGlDMWViY1dZTXAyODVaYnF4UjF1eU1UL2E1TmdpZDlJYTFkbk92cTRr?=
 =?utf-8?B?STFOVDl0Um1qUCtXUVVWMDhlNWt4WE9adXhtaDU2Syt4YWM1cTBKNUFlc01j?=
 =?utf-8?B?VGVKQTdrRkdJVi9tZU0rbUsyZFJ4SXRScWxKeHVuU0t4Ym12Ly8yYWNObzBF?=
 =?utf-8?B?bzlBSWlTdmErNWM3UHVXTnhIYXJzOXR5dWVIOFRUZyt4V0VQYnU0bWpFQUlE?=
 =?utf-8?B?ZXZmUkE0UFdxQndLTnlJSElPTTJ2ZlgvMHovOXFQcUU0S2NlRHhPUTkrMFZY?=
 =?utf-8?B?YjhORm4zRmdIVVMzU0xpMjUxdGpBTU90NUxtTG9sWlB3Ulk5SjQrWEhIQ0dT?=
 =?utf-8?B?NXRIZ2dMZUZFbUtVZEhicEZpNTVrTk9Da0dkTDNKajU5SllTN0NNdVg4UVQr?=
 =?utf-8?B?cHZGbC96d1NIYjdnSjMrUUxyb2h1a25WcE5Ld1M4TFZGR29ZVVpHc3dVc1B0?=
 =?utf-8?Q?nnByL/zIPAyfvkX/HLfZy1fnAnuK78FM4JlFqEk?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 5ea1d46f-ede4-47f1-c5fe-08d968b055f5
X-MS-Exchange-CrossTenant-AuthSource: CO1PR11MB4914.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Aug 2021 16:41:21.7857
 (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: v0BjUbWb+D18V+CX0Elm1EKzlbQmMLSkzO5XUKkFI8G6pAVPyjaMeO92gYinphfxx+x9rl6oUWxDa1L3Ekj5rGhZT6SVyYqFd9ciuvMVup4=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR11MB2000
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 8/26/2021 9:18 AM, Eric Dumazet wrote:

>> +static inline int ixgbe_determine_xdp_q_idx(int cpu)
>> +{
>> +	if (static_key_enabled(&ixgbe_xdp_locking_key))
>> +		return cpu % IXGBE_MAX_XDP_QS;
>> +	else
>> +		return cpu;
> 
> Even if num_online_cpus() is 8, the returned cpu here could be
> 
> 0, 32, 64, 96, 128, 161, 197, 224
> 
> Are we sure this will still be ok ?

I'm not sure about that one myself. Jason?

> 
>> +}
>> +
>>  static inline u8 ixgbe_max_rss_indices(struct ixgbe_adapter *adapter)
>>  {
>>  	switch (adapter->hw.mac.type) {
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c
>> index 0218f6c..884bf99 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c
>> @@ -299,7 +299,10 @@ static void ixgbe_cache_ring_register(struct ixgbe_adapter *adapter)
>>  
>>  static int ixgbe_xdp_queues(struct ixgbe_adapter *adapter)
>>  {
>> -	return adapter->xdp_prog ? nr_cpu_ids : 0;
>> +	int queues;
>> +
>> +	queues = min_t(int, IXGBE_MAX_XDP_QS, num_online_cpus());
> 
> num_online_cpus() might change later...

I saw that too, but I wonder if it doesn't matter to the driver. If a
CPU goes offline or comes online after the driver loads, we will use
this logic to try to pick an available TX queue. But this is a
complicated thing that is easy to get wrong, is there a common example
of how to get it right?

A possible problem I guess is that if the "static_key_enabled" check
returned false in the past, we would need to update that if the number
of CPUs changes, do we need a notifier?

Also, now that I'm asking it, I dislike the global as it would apply to
all ixgbe ports and each PF would increment and decrement it
independently. Showing my ignorance here, but I haven't seen this
utility in the kernel before in detail. Not sure if this is "OK" from
multiple device (with the same driver / global namespace) perspective.