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: <1b747e11-d358-52aa-0dfc-e795a8949106@suse.com>
Date:   Mon, 24 Oct 2022 14:37:43 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.12.0
Subject: Re: [PATCH v2 2/2] module: Merge same-name module load requests
Content-Language: en-US
To:     Prarit Bhargava <prarit@redhat.com>,
        Luis Chamberlain <mcgrof@kernel.org>
Cc:     pmladek@suse.com, linux-modules@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20220919123233.8538-1-petr.pavlu@suse.com>
 <20220919123233.8538-3-petr.pavlu@suse.com>
 <YzdR0gRNQI2BGnJ9@bombadil.infradead.org>
 <aa8d9456-b260-d999-0296-8e6ab876af7a@suse.com>
 <Y07xX2ejlg0oFoEy@bombadil.infradead.org>
 <2342ef17-f8f9-501f-0f7b-5a69e85c2cf4@redhat.com>
From:   Petr Pavlu <petr.pavlu@suse.com>
In-Reply-To: <2342ef17-f8f9-501f-0f7b-5a69e85c2cf4@redhat.com>
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?YTkwNTNhOUpBSEtBWXdseXJ6YjdTK2QzbktPd1hlaUd1dGMrMmc1TVNqSjhp?=
 =?utf-8?B?YTRUaWs1VUc0S3FLa3M0VzNKdFBJRTlIUU03M1grc20ySjZGbEVETnYzbVVa?=
 =?utf-8?B?UDVZUkdwN1o1V1F6V3RHWWZJWnlML24rVTNzaTdRTTg2Ty9XdE85OWFnSFhr?=
 =?utf-8?B?OGZBMmc2S0R5bXJ2WDZaYXBnMEZQZUlVdGovNEpFbXk2M3hxY1FSNUNKUit6?=
 =?utf-8?B?ZTcxbHZUTnRqZGErVmE3OXFoS3p5MTBmRlJlbG5XWXVpM0I0L2VXU1MveWp1?=
 =?utf-8?B?R3QzcGZmU3JScmVCQ0lGcUhWQzQ0YXhKYUZLOWRqZEtqS21ZaEpGSXBGNm5N?=
 =?utf-8?B?SkFmZXRlc1cxVWVnTjg0NklzdVFGdjJLM3Z5eXRDem9QMHp6V1ducDdlTjBZ?=
 =?utf-8?B?aDZISTZYcmV3aTZxMTBxQ0JUK3hWVnVFRnpTS0JWN2VoeXJoTkJaVVk1Tm5s?=
 =?utf-8?B?QitKM2lTd3dVU1UyMjgxYnIxdDFhMHh1VW01UHczSFo3cW5CU3RFOC9xYUVU?=
 =?utf-8?B?anN5RmVVMUNXZ2FiMjhBVDArWFUxRlovekhUNnNxLzlxaWFEZ1paYjArYjFw?=
 =?utf-8?B?Z3NWTW05Zk9Lb1VOTlhxcHFySlFmb3BGWTdCMk04YXpzV1JKaXg1WnkwQmRz?=
 =?utf-8?B?MzUyejgya1NNbnpMMkVhZTl6MXllbThvSVV1RFJzcG5wdDFXTldxVlJDa0lL?=
 =?utf-8?B?NXdKRnNrSVVySVlsSVgrZWRXRS92MDBFeHpITkVlcHltVUhiMnRsN3pWdGkr?=
 =?utf-8?B?UDNDK3U2cjVTWmtxd0l0SGRZb2xHUzE4MlFydlFOeG9reFc4SkdjZkZldnJl?=
 =?utf-8?B?emlJVjBVRXo1bjJLUlpDRjVQdHlBVURSRU52aUZYVEp1Z2Q5aEc0eTUxamhh?=
 =?utf-8?B?dDFmVFJ4bEdvNTFWTGlRTWVmaUtVT1N6RXhzQ1FITkJ5cnBtK05PaEJ5Wlhr?=
 =?utf-8?B?MzNLOGhSSGE1OXUvVkQzeTZXYUJuS0dxaGJQMy9NbGNoRk9xMjJ6VDdzTmdl?=
 =?utf-8?B?dEh5VDdvdUVNYm9mQVoyZ0lnajNsMHBEMXNYWDBNcnpKYmNpdk1lTnpCWGpj?=
 =?utf-8?B?M0duN0JuR1BSM0pXN3J1RTVibW1wRzViU0xlaVd1NGlVUWpZZFhnRmg1VmZE?=
 =?utf-8?B?VzkyNkpwNVlvNG9vR0MrRlV2KzgwWUFWcWZtOVcyQngrbHZYNTVjWUVQOEwv?=
 =?utf-8?B?K2tnTVNlYnBCbnRqdlNsNm5BcDRIY3NYR1pDSXpzNFFxZ0l3VFlyQjRWeTBn?=
 =?utf-8?B?MGl0czcvc2VPNDFFRURDU3dUMTh0QVYxMGZUanFwMHEvOVRveTkrQWowNG1h?=
 =?utf-8?B?bWwxdnQxWEdYOUVzb2lYcnJXQXEvR0RXZ3g0R0RpaW1tR1NxUS9LU2pHdzd2?=
 =?utf-8?B?RXBZUXR4aW9TZTNkWmtYOE5kV3JVRnR6bTBkeGhha2Rrb25ocmRYRkQ5TTlO?=
 =?utf-8?B?UDJ0Y215bDJhTnlKcHNaNk1mSXFMOUxTSWFyQjFzemRpdERieVFEVk0zVjJl?=
 =?utf-8?B?OUE3b1BTN1plSjdiMGxobDVxVGJEZjRKSDM1VHlvZkpTSnZEY25xTkxJU3Zv?=
 =?utf-8?B?d2NuTXJxb2Zvby9aNHlNcU1ZemJ3T1FEU3hObXJ6dGpqRjZxakhza2NNdEFp?=
 =?utf-8?B?bXJzMTBZTWFocmV5bVdUR3dOQm9TSWZYelU0SDkzWjl1WEZJQmF0djZ0eC8r?=
 =?utf-8?B?cEk4WlFFUnJzVzRrM3FIR21NWXJxdGxhbkRFN0xFcXRUZTVNYlZNb0ZpbG5R?=
 =?utf-8?B?Vlo0UXJhRzRzOUR6VVlnUytTd1IxYnE2L3Y4eXR2b0dDLzczaVVBS3lrTFJp?=
 =?utf-8?B?THJ4b2dPM0JBR1JMTnFvd0x6MXBjMjZXeFhRVGh6QjYzbWRDWmVrNi8vTlM1?=
 =?utf-8?B?Yml0dUJ3aUgwV05tUkZwTnlvVmZ3NVMydVFyL1lNYmNhVmZlUmVONGNLVFhS?=
 =?utf-8?B?NWlTYTlrL3NVR3hBNU95eko5aDZKVWxLNjk2WGJsMVEzWEVXODlucVRPb0dZ?=
 =?utf-8?B?dGp1eG90dzM2WnBpSEozSTdiSTBQbWpVMEdmWTVKN29BVWpZOER3SEdNbXht?=
 =?utf-8?B?M081Q0Z0dm84Z2JQSXNpejg1V0dyQ1d1R2dHN1JPWVc2Z3RCTkxXK1hkbFM5?=
 =?utf-8?Q?CqLA5MbEwiLdxmbexak643/1D?=
X-OriginatorOrg: suse.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 2a2dbf83-b4e6-43b3-2767-08dab5bc8d32
X-MS-Exchange-CrossTenant-AuthSource: AM0PR0402MB3395.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 24 Oct 2022 12:37:45.5731
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: f7a17af6-1c5c-4a36-aa8b-f5be247aa4ba
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 9W/Wa3M8MuvqSc36kEI7P+Ukyov+PMklzMKwwiEcjHYViegEYoZ2PDX8VtiTXLQDmJq6J9tu52w+d3xw8GcOow==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR04MB6926
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        SPF_HELO_PASS,SPF_PASS 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

On 10/18/22 21:53, Prarit Bhargava wrote:
> Quoting from the original thread,
> 
>>
>> Motivation for this patch is to fix an issue observed on larger machines with
>> many CPUs where it can take a significant amount of time during boot to run
>> systemd-udev-trigger.service. An x86-64 system can have already intel_pstate
>> active but as its CPUs can match also acpi_cpufreq and pcc_cpufreq, udev will
>> attempt to load these modules too. The operation will eventually fail in the
>> init function of a respective module where it gets recognized that another
>> cpufreq driver is already loaded and -EEXIST is returned. However, one uevent
>> is triggered for each CPU and so multiple loads of these modules will be
>> present. The current code then processes all such loads individually and
>> serializes them with the barrier in add_unformed_module().
>>
> 
> The way to solve this is not in the module loading code, but in the udev 
> code by adding a new event or in the userspace which handles the loading 
> events.
> 
> Option 1)
> 
> Write/modify a udev rule to to use a flock userspace file lock to 
> prevent repeated loading.  The problem with this is that it is still 
> racy and still consumes CPU time repeated load the ELF header and, 
> depending on the system (ie a large number of cpus) would still cause a 
> boot delay.  This would be better than what we have and is worth looking 
> at as a simple solution.  I'd like to see boot times with this change, 
> and I'll try to come up with a measurement on a large CPU system.

It is not immediately clear to me how this can be done as a udev rule. You
mention that you'll try to test this on a large CPU system. Does it mean that
you have a prototype implemented already? If yes, could you please share it?

My reading is that one would need to update the "MODALIAS" rule in
80-drivers.rules [1] to do this locking. However, that just collects
'kmod load' (builtin) for udev to execute after all rules are processed. It
would then be required to synchronize udev workers to prevent repeated
loading?

> Option 2)
> 
> Create a new udev action, "add_once" to indicate to userspace that the 
> module only needs to be loaded one time, and to ignore further load 
> requests.  This is a bit tricky as both kernel space and userspace would 
> have be modified.  The udev rule would end up looking very similar to 
> what we now.
> 
> The benefit of option 2 is that driver writers themselves can choose 
> which drivers should issue "add_once" instead of add.  Drivers that are 
> known to run on all devices at once would call "add_once" to only issue 
> a single load.

On the device event side, I more wonder if it would be possible to avoid tying
up cpufreq and edac modules to individual CPU devices. Maybe their loading
could be attached to some platform device, even if it means introducing an
auxiliary device for this purpose? I need to look a bit more into this idea.

[1] https://github.com/systemd/systemd/blob/4856f63846fc794711e1b8ec970e4c56494cd320/rules.d/80-drivers.rules

Thanks,
Petr