Re: [PATCH v4 1/2] x86/mem_sharing: make fork_reset more configurable

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Mon, Apr 25, 2022 at 8:53 AM George Dunlap <dunlapg@xxxxxxxxx> wrote:
>
>
>
> On Mon, Apr 25, 2022 at 12:29 PM Tamas K Lengyel <tamas@xxxxxxxxxxxxx> wrote:
>>
>>
>>
>> On Mon, Apr 25, 2022, 3:49 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>>
>>> On 22.04.2022 16:07, Tamas K Lengyel wrote:
>>> > On Wed, Apr 13, 2022 at 9:43 AM Tamas K Lengyel <tamas.lengyel@xxxxxxxxx> 
>>> > wrote:
>>> >>
>>> >> Allow specify distinct parts of the fork VM to be reset. This is useful 
>>> >> when a
>>> >> fuzzing operation involves mapping in only a handful of pages that are 
>>> >> known
>>> >> ahead of time. Throwing these pages away just to be re-copied 
>>> >> immediately is
>>> >> expensive, thus allowing to specify partial resets can speed things up.
>>> >>
>>> >> Also allow resetting to be initiated from vm_event responses as an
>>> >> optimization.
>>> >>
>>> >> Signed-off-by: Tamas K Lengyel <tamas.lengyel@xxxxxxxxx>
>>> >
>>> > Patch ping. Could I get a Reviewed-by if there are no objections?
>>>
>>> Hmm, this is a little difficult. I'd be willing to give an ack, but that's
>>> meaningless for most of the code here. Besides a stylistic issue I did
>>> point out which I'm not happy with, I'm afraid I'm not good enough at
>>> mem-sharing and forking. Therefore I wouldn't want to offer an R-b.
>>> Considering the VM event interaction, maybe the BitDefender guys could
>>> take a stab?
>>>
>>> Of course you'd then still need a tool stack side ack.
>>
>>
>> So my take is that noone cares about mem_sharing, which is fine, its an 
>> obscure experiment subsystem.
>
>
> My take is slightly different; it's more that the project is large enough 
> that it's difficult to se where the needs are.  If Roger or Andy or I or Wei 
> or anyone see a thread with you & Jan going back and forth, it's natural for 
> us to assume that you & Jan have it in hand, and there's no need for us to 
> read through the thread.  Jan dislikes asking specific people for a review, 
> but many of the rest of us have sort of gotten in the habit of doing so, as a 
> way to solve the "visibility" issue.  The only other way I can think of to 
> solve the problem is to have a robot try to assign tasks to people -- a 
> method that has received skepticism, and would also require a non-negligible 
> amount of tooling to be written.

What's the point of the MAINTAINER's file and being automatically CC-d
if you then still separately have to ping the same people by name?
It's fine not to give an a-b/r-b if you still see discussion on some
parts of the patch, but like on this one, where the tools changes are
trivial - why would you wait? To be frank I long consider the
tools-side part of Xen unmaintained with only the most trivial stuff
ever having a chance to make it in. VM forking has effectively 0
toolstack side support in-tree because I never got any feedback from
tools maintainers after sending the patches in for months. I would
consider the toolstack side stuff in this patch for example trivial,
but again, no tools maintainers ever look at it so I was actually
considering dropping it from the patch completely since I really only
need the vm_event interface. Again, that would be dropping an
otherwise potentially useful interface purely due to the dysfunction
of the project's maintenance.

>
>>
>> But the only path I see as maintainer to get anything in-tree is if I hand 
>> the task of writing the patch to a coworker who then sends it in so that I 
>> can ack it. This is clearly disfunctional and is to the detriment of the 
>> project overall. We need to get some rules in place to avoid situations like 
>> this that clearly lead to no development and no improvement and a huge 
>> incentive to forgot about upstreaming. With no substantive objections but no 
>> acks a maintainer should be able to get changes in-tree. That's part of what 
>> I would consider maintaining a codebase to be!
>
>
> Another possibility would be to ask your colleague actually do a Reviewed-by. 
>  The first time or two they might not be "of suitable stature in the 
> community", but I don't think it should take long to establish such a 
> stature, if they were doing the review in earnest.

Sure, but clearly still more effort then it should be just to work
around the system.

>
> I do agree that it seems like in this situation, the bar seems too high for 
> you to get your own code checked in.  I'd be open to the argument that we 
> should change the text of the check-in policy in MAINTAINERS to allow 
> maintainer modifications with only an Acked-by.

Happy to hear! I think such a change would reduce the overhead on
reviewing patches like this that clearly have no effect on anything
else.

>
>>
>> Anyway, to be realistic I don't expect that option to materialize so I'm 
>> very close to just stop all contributions to the project. It's dishartening.
>
>
>
> I can understand why you'd be disheartened if you thought you just couldn't 
> get any code checked in even as maintainer.  However, there are lots of 
> escalation paths open to you: you could email the community manager (me); you 
> could make a wider appeal on IRC for reviewers; you could raise the general 
> issue at the community call; you could send a patch proposing changes to the 
> check-in procedure described in MAINTAINERS.

Fair point. But when your main job is not working on Xen and you have
a couple weeks in-between other stuff to try to get some improvements
in, it's not really viable to have to go reform the whole project.
That's just the reality. If we can reduce the bar on getting code
upstream in situations like this then I would be happy to continue
working on the project but otherwise I don't see how this is worth
anyone's time.

Tamas