Monday Patch Queue Review update (2023-04-03)

Monday Patch Queue Review update (2023-04-03)

[Carlos O'Donell]

Most recent meeting status is always here:
https://sourceware.org/glibc/wiki/PatchworkReviewMeetings#Update

Meeting: 2023-04-03 @ 0900h EST5EDT

Video/Audio: https://bbb.linuxfoundation.org/room/adm-alk-1uu-7fu

IRC: #glibc on OFTC.

Review new patches and restart review at the top.

 * State NEW delegate NOBODY at 248 patches.
 * Carlos' SLI at 156 days.
 * https://sourceware.org/bugzilla/show_bug.cgi?id=30301 (memalign improvement introduced regressions)
  * Discussed what we need to do to fix the issue.
 * Adhemerval: Raised the issue of submitted bugs for the upcoming release?
  * Required? Desired? Useful? Filtered?
  * Carlos: Unless someone asks for a shortened NEWS list, I'd leave these there as an artifacts of showing development in progress.
  * Florian: Notes that Version field in bugzilla includes 2 upcoming versions so that bugs in devel can be tracked correctly for pre-release based on date.
 * Started at 67211
 * LoongArch: Ensure consistency with kernel by using union for struct members in mcontext_t and ucontext_t. (Caiyin Yu)
  * Noted that these changes seem odd given that any implementation can pick names as required.
 * [v2,18.2/34] hurd: Port trampoline.c to x86_64 (Sergey)
 * [v2,18.1/34] hurd: Do not declare local variables volatile (Sergey)
 * [v2] hurd: Implement sigreturn for x86_64 (Sergey)
 * x86/dl-cacheinfo: remove unsused parameter from handle_amd (Andreas)
  * Needs review from anyone. Fairly generic cleanup.
 * implement dlmem() function (stsp)
  * Carlos to comment again about dlmem().
 * build-many-glibcs.py: --disable-gcov for gcc-first (Jan-Benedict Glaw)
  * Discussed briefly to disable gcov.
  * Carlos: I'll look at this briefly to commit.
 * https://sourceware.org/bugzilla/show_bug.cgi?id=30293
  * Needs review in light of the glibc implementaiton.
 * [v2] Add tests for strdup and strndup (BZ #30266) (Joe)
  * Adhemerval reviewing test additions.
 * [v5,1/1] Created tunable to force small pages on stack allocation. (Cupertino)
  * Adhemerval to review.
  * Carlos: This seems like we should be doing this by default?
  * Florian: Guard page size could be 2MiB
  * Carlos: Split the tcb from stack?
  * Florian: Note that the reported RSS increase is not technically real because it's untouched thread stack.
  * Adding a tunable is a good first step.
 * [v3] elf: Fix slow tls access after dlopen [BZ #19924] (Szabolcs)
  * Still needs review. Important for the release.
 * [v2] manual: Document __wur usage under _FORTIFY_SOURCE (Siddhesh)
  * Florian to review.
 * Add clone3 support for multiple architectures (Adhemerval)
  * Carlos to review.
 * Stopped at 66592.

-- 
Cheers,
Carlos.

Re: Monday Patch Queue Review update (2023-04-03)

[Sergey Bugaev]

Hi,

> * [v2,18.2/34] hurd: Port trampoline.c to x86_64 (Sergey)
> * [v2,18.1/34] hurd: Do not declare local variables volatile (Sergey)
> * [v2] hurd: Implement sigreturn for x86_64 (Sergey)

could you please explain, what do I make of this?

Does this just mean that my patches have been received? Reviewed? That
they're on someone's review queue? Or what?

Do I need to take any action?

Sergey

Re: Monday Patch Queue Review update (2023-04-03)

[Carlos O'Donell]

On 4/3/23 16:30, Sergey Bugaev wrote:
> Hi,
> 
>> * [v2,18.2/34] hurd: Port trampoline.c to x86_64 (Sergey)
>> * [v2,18.1/34] hurd: Do not declare local variables volatile (Sergey)
>> * [v2] hurd: Implement sigreturn for x86_64 (Sergey)
> 
> could you please explain, what do I make of this?

Great question! :-)

> Does this just mean that my patches have been received? Reviewed? That
> they're on someone's review queue? Or what?

Your patches:

(a) Made it to the mailing list.

(b) Made it into Patchwork (which we use for patch tracking)

(c) Were reviewed as part of the weekly patch queue review.

    - We look over patches in the meeting to see if we can help
      move them forward.

(d) Did not get assigned any specific reviewer to review them.

    - This happens for any number of reasons e.g. lack of a person
      who feels qualified to review a subsystem or machine,
      lack of hardware to test, etc.

The outcome of the meeting was that we didn't find a way to help
move your patches forward, sorry for that.

Your personal queue is here:

https://patchwork.sourceware.org/project/glibc/list/?submitter=35358

Please have look over the queue to see if some of those patches have
been committed or could have their state updated.

> Do I need to take any action?

Nothing more than the usual task of pinging the patch on the list
asking for review.

Thomas Schwinge and Samuel Thibault are the GNU Hurd OS maintainers
so we often defer to them to review Hurd-specific patches.

-- 
Cheers,
Carlos.

Re: Monday Patch Queue Review update (2023-04-03)

[Sergey Bugaev]

On Tue, Apr 4, 2023 at 8:48 PM Carlos O'Donell <carlos@redhat.com> wrote:
> Your patches:
>
> (a) Made it to the mailing list.
>
> (b) Made it into Patchwork (which we use for patch tracking)
>
> (c) Were reviewed as part of the weekly patch queue review.
>
>     - We look over patches in the meeting to see if we can help
>       move them forward.
>
> (d) Did not get assigned any specific reviewer to review them.
>
>     - This happens for any number of reasons e.g. lack of a person
>       who feels qualified to review a subsystem or machine,
>       lack of hardware to test, etc.

Thanks for the explanation!

> The outcome of the meeting was that we didn't find a way to help
> move your patches forward, sorry for that.

Ah, well, based on my previous experience, we just have to patiently
wait for Samuel to find some time to review the patches :)

> Your personal queue is here:
>
> https://patchwork.sourceware.org/project/glibc/list/?submitter=35358
>
> Please have look over the queue to see if some of those patches have
> been committed or could have their state updated.

Cool -- but I see that Patchwork gets confused by my somewhat liberal
use of patch series formatting:

* "[v2] hurd: Add expected abilist files for x86_64" supersedes
  "[RFC,34/34] hurd: Add expected abilist files for x86_64", so the
  latter should have State = Superseded, and the former State = RFC

* The same goes for "[v2] hurd: Implement sigreturn for x86_64" and
  "[RFC,32/34] hurd: Implement sigreturn for x86_64"

* Ditto for the "Alignment-respecting x86_64 trampoline.c"
  mini-series, which collectively supersedes
  "[RFC,18/34] hurd: Port trampoline.c to x86_64"

(but it did grok that [PATCH v2 18.0/34] means a cover letter! unless
that was done manually)

Is there anything I should do differently when sending a replacement
for a patch (but not a v2 of the whole series) to make it easier for
Patchwork to understand what's going on?

Sergey

Re: Monday Patch Queue Review update (2023-04-03)

[Carlos O'Donell]

On 4/4/23 15:09, Sergey Bugaev wrote:
> On Tue, Apr 4, 2023 at 8:48 PM Carlos O'Donell <carlos@redhat.com> wrote:
>> Your patches:
>>
>> (a) Made it to the mailing list.
>>
>> (b) Made it into Patchwork (which we use for patch tracking)
>>
>> (c) Were reviewed as part of the weekly patch queue review.
>>
>>     - We look over patches in the meeting to see if we can help
>>       move them forward.
>>
>> (d) Did not get assigned any specific reviewer to review them.
>>
>>     - This happens for any number of reasons e.g. lack of a person
>>       who feels qualified to review a subsystem or machine,
>>       lack of hardware to test, etc.
> 
> Thanks for the explanation!
> 
>> The outcome of the meeting was that we didn't find a way to help
>> move your patches forward, sorry for that.
> 
> Ah, well, based on my previous experience, we just have to patiently
> wait for Samuel to find some time to review the patches :)
> 
>> Your personal queue is here:
>>
>> https://patchwork.sourceware.org/project/glibc/list/?submitter=35358
>>
>> Please have look over the queue to see if some of those patches have
>> been committed or could have their state updated.
> 
> Cool -- but I see that Patchwork gets confused by my somewhat liberal
> use of patch series formatting:
> 
> * "[v2] hurd: Add expected abilist files for x86_64" supersedes
>   "[RFC,34/34] hurd: Add expected abilist files for x86_64", so the
>   latter should have State = Superseded, and the former State = RFC

RFC and Superseded markup is manual today.

Looking at libc-alpha I see the following nesting:

[RFC PATCH glibc 34/34] hurd: Add expected abilist files for x86_64   Sergey Bugaev
    [RFC PATCH glibc 34/34] hurd: Add expected abilist files for x86_64   Florian Weimer
        [PATCH v2] hurd: Add expected abilist files for x86_64   Sergey Bugaev
        [PATCH v2] hurd: Add expected abilist files for x86_64   Florian Weimer 

Patchwork considers the "[PATCH v2] hurd: ..." to be distinct patches.

I believe that if you post with the subject e.g. git format-patch --rfc -v2:

"[RFC PATCH v2 34/34] ..."

Then it may update the patch to v2, but I haven't confirmed that.

> * The same goes for "[v2] hurd: Implement sigreturn for x86_64" and
>   "[RFC,32/34] hurd: Implement sigreturn for x86_64"

Likewise.

> * Ditto for the "Alignment-respecting x86_64 trampoline.c"
>   mini-series, which collectively supersedes
>   "[RFC,18/34] hurd: Port trampoline.c to x86_64"

I've never seen mini-series formatting like this. Is it used somewhere else?

> (but it did grok that [PATCH v2 18.0/34] means a cover letter! unless
> that was done manually)

It does that manually for the 0th entry in a series.

> Is there anything I should do differently when sending a replacement
> for a patch (but not a v2 of the whole series) to make it easier for
> Patchwork to understand what's going on?

I suggest trying my comments above and we'll see if that works.

I do not suggest using a mini-series. If you have to do a mini-series I think it
would be better to post a v2 with a renumbered set of patches.

-- 
Cheers,
Carlos.