Re: [PATCH] io_uring/napi: remove duplicate io_napi_entry timeout assignation

[Jens Axboe <axboe@kernel.dk>]

On 8/13/24 6:09 PM, Olivier Langlois wrote:
> On Tue, 2024-08-13 at 12:35 -0600, Jens Axboe wrote:
>> On 8/13/24 11:22 AM, Olivier Langlois wrote:
>>> On Mon, 2024-08-12 at 14:40 -0600, Jens Axboe wrote:
>>>>
>>>>
>>>>> 3. I am surprised to notice that in __io_napi_do_busy_loop(),
>>>>> list_for_each_entry_rcu() is called to traverse the list but
>>>>> the
>>>>> regular methods list_del() and list_add_tail() are called to
>>>>> update
>>>>> the
>>>>> list instead of their RCU variant.
>>>>
>>>> Should all just use rcu variants.
>>>>
>>>> Here's a mashup of the changes. Would be great if you can test -
>>>> I'll
>>>> do
>>>> some too, but always good with more than one person testing as it
>>>> tends
>>>> to hit more cases.
>>>>
>>> Jens,
>>>
>>> I have integrated our RCU corrections into
>>> https://lore.kernel.org/io-uring/5fc9dd07e48a7178f547ed1b2aaa0814607fa246.1723567469.git.olivier@trillion01.com/T/#u
>>>
>>> and my testing so far is not showing any problems...
>>> but I have a very static setup...
>>> I had no issues too without the corrections...
>>
>> Thanks for testing, but regardless of whether that series would go in
>> or
>> not, I think those rcu changes should be done separately and upfront
>> rather than be integrated with other changes.
>>
> sorry about that...
> 
> I am going to share a little bit how I currently feel. I feel
> disappointed because when I reread your initial reply, I have not been
> able to spot a single positive thing said about my proposal despite
> that I have prealably tested the water concerning my idea and the big
> lines about how I was planning to design it. All, I have been told from
> Pavel that the idea was so great that he was even currently playing
> with a prototype around the same concept:
> https://lore.kernel.org/io-uring/1be64672f22be44fbe1540053427d978c0224dfc.camel@trillion01.com/T/#mc7271764641f9c810ea5438ed3dc0662fbc08cb6

Sorry if I made you feel like that, was not my intent. I just latched on
to the ops setup and didn't like that at all. I do think
pre-registrering an ID upfront is a good change, as it avoids a bunch of
hot path checking. And for a lot of use cases, single NAPI instance per
ring is indeed the common use case, by far.

Please realize that I get (and answer) a lot of email and also have a
lot of other things on my plate than answer emails, hence replies can
often be short and to the point. If I don't mention a part of your
patch, I either missed it or figured it necessitated a rework based on
the other feedback.

> you also have to understand that all the small napi issues that I have
> fixed this week are no stranger from me working on this new idea. The
> RCU issues that I have reported back have been spotted when I was doing
> my final code review before testing my patch before submitting it.
> 
> keep in mind that I am by far a git magician. I am a very casual
> user... Anything that is outside the usual beaten trails such as
> reordoring commits or breaking them down feels perilious to me...

Reviews and fixes are _always_ appreciated, even if emails don't include
that. They sure could, but then it'd just be a copied blurb, and I don't
think that's very genuine. FWIW, your patch submissions have been fine.
The critique I had for your patch was that you should not include little
fixes with larger changes. That's just not how kernel patches are done.
You do the little fixes first, and then the bigger change on top as
separate changes. The reason for that are two-fold: it makes it easier
to backport them to stable, if that is needed, and it makes it easier to
review the actual functional change. Both of those are really important.

> I had 230+ lines changes committed when you confirmed that few lines
> should be changed to address this new RCU issue. I did figure that it
> would not that big a deal to include them with the rest of my change.

See above, it is a big deal, and honestly something I recommend for any
kind of project, not just the kernel.

> that being said, if my patch submission is acceptable conditional to
> needed rework, I am willing to learn how to better use git to meet your
> requirements.

Your git skills are fine, don't worry about that, there's nothing wrong
with the mechanics of the submission. It's just the changes themselves
that need a splitting up, and rework.

-- 
Jens Axboe