ReviewGPT

Posted Jun 26, 2025 17:03 UTC (Thu) by adobriyan (subscriber, #30858)
Parent article: Supporting kernel development with large language models

> -static struct hlist_head event_hash[EVENT_HASHSIZE] __read_mostly;
> +static DEFINE_HASHTABLE(event_hash, EVENT_HASH_BITS);

@sashal

Restore "event_hash" section placement removed in commit 391dda1bd7c56de62b96126214f040fe8965561b.

Use different DEFINE_HASHTABLE macro variant to do that.

Send the fix to linux-kernel mailing list, Cc Linux tracing subsystem maintainers.

Mention it is minor fix for commit 391dda1bd7c56de62b96126214f040fe8965561b.

ReviewGPT

Posted Jun 26, 2025 19:28 UTC (Thu) by comex (subscriber, #71521) [Link] (2 responses)

(Disclaimer: I am not sashal.)

…In other words, you're saying that the patch is buggy because it drops the __read_mostly attribute (which places the data in a different section).

That's a good reminder of how untrustworthy LLMs still are. Even for such a simple patch, the LLM was still able to make a subtle mistake.

To be fair, a human could definitely make the same mistake. And whatever humans reviewed this patch apparently did miss the mistake. But I think a human would be at least less likely to make the mistake in the first place.

Also, it's not a *major* mistake. __read_mostly is just a performance hint. It shouldn't make much difference for a large structure like event_hash, since only the start and end of it can possibly share a cache line with other variables. Dropping __read_mostly might even be an improvement, given the relatively-recent guidelines to "be mindful and selective of [__read_mostly's] use" and limit it to "tightly packed" data [1], which a large sparse hash table is not.

Still, if dropping __read_mostly had been intentional, the commit message would have mentioned it. Since it doesn't, this is a mistake all right.

[1] https://github.com/torvalds/linux/blob/e34a79b96ab9d49ed8...

ReviewGPT

Posted Jun 26, 2025 19:40 UTC (Thu) by adobriyan (subscriber, #30858) [Link]

Readmostliness and API conversion are separate issues.

DEFINE_READ_MOSTLY_HASHTABLE exists and LLM missed it.

If someone knows other LLM commits, post them, we'll see what they are worth.

ReviewGPT

Posted Jun 29, 2025 15:47 UTC (Sun) by nevets (subscriber, #11875) [Link]

As the maintainer that pulled Sasha's patch, I missed the removal of the "__read_mostly" because I thought Sasha had written it, and that's not something that he would have lightly removed without mentioning it.

I first thought that's even a bug as the hash is set up at boot or module load or unload and doesn't get changed other than that. But it likely could be removed because it's only for the trace output and that's not performance critical. But it should have been a separate patch.