[PATCH v2 2/3] Linux: Add the sched_setattr and sched_getattr functions

[enh]

On Fri, Sep 6, 2024 at 2:33 PM Carlos O'Donell <carlos@redhat.com> wrote:
>
> On 9/6/24 9:58 AM, Florian Weimer wrote:
> > * Adhemerval Zanella Netto:
> >
> >> On 06/09/24 10:26, Florian Weimer wrote:
> >>> * Adhemerval Zanella Netto:
> >>>
> >>>> On 05/09/24 17:24, Florian Weimer wrote:
> >>>>> And struct sched_attr.
> >>>>>
> >>>>> In sysdeps/unix/sysv/linux/bits/sched.h, the hack that defines
> >>>>> sched_param around the inclusion of <linux/sched/types.h> is quite
> >>>>> ugly, but the definition of struct sched_param has already been
> >>>>> dropped by the kernel, so there is nothing else we can do and maintain
> >>>>> compatibility of <sched.h> with a wide range of kernel header
> >>>>> versions.  (An alternative would involve introducing a separate header
> >>>>> for this functionality, but this seems unnecessary.)
> >>>>>
> >>>>> The existing sched_* functions that change scheduler parameters
> >>>>> are already incompatible with PTHREAD_PRIO_PROTECT mutexes, so
> >>>>> there is no harm in adding more functionality in this area.
> >>>>>
> >>>>> The documentation mostly defers to the Linux manual pages.
> >>>>
> >>>> So I take that the defined approach for syscalls that accepts extensible
> >>>> struct arguments would just to document that it can not be used on places
> >>>> that might affect the ABI, include the kernel definitions if present,
> >>>> and just pass the size to the kernel then?
> >>>>
> >>>> I was not sure how to proceed with the openat2 support back when I first
> >>>> proposed [1] because I felt to me that we did not fully agreed how to
> >>>> handle this cases.
> >>>>
> >>>> [1] https://sourceware.org/pipermail/libc-alpha/2024-April/156319.html
> >>>
> >>> See the parallel discussion with enh.  I don't think it's a big problem.
> >>>
> >>> We already have struct statx in the installed headers, which behaves
> >>> pretty much the same way, except that extensions are currently using up
> >>> the padding space.
> >>
> >> The statx is slight different wrt to kABI because it does not really
> >> follow the current behavior or adding the size for the syscall (and
> >> I recall some kernel developers to regret this decision).
> >
> > Hmm, right.  I think it was expected that it was controlled by the
> > flags.  In any case, the struct is expected to grow additional fields in
> > the future, too.
>
> It *is* controlled by the flags.
>
> And there is an expansion mechanism:
>
> 159 #define STATX__RESERVED         0x80000000U     /* Reserved for future struct statx expansion */
>
> The behaviour is not like sched_setattr/sched_getattr with size though.
>
> e.g.
>
>  62 /*
>  63  * Structures for the extended file attribute retrieval system call
>  64  * (statx()).
>  65  *
>  66  * The caller passes a mask of what they're specifically interested in as a
>  67  * parameter to statx().  What statx() actually got will be indicated in
>  68  * st_mask upon return.
>  69  *
>  70  * For each bit in the mask argument:
>  71  *
>  72  * - if the datum is not supported:
>  73  *
>  74  *   - the bit will be cleared, and
>  75  *
>  76  *   - the datum will be set to an appropriate fabricated value if one is
>  77  *     available (eg. CIFS can take a default uid and gid), otherwise
>  78  *
>  79  *   - the field will be cleared;
>  80  *
>  81  * - otherwise, if explicitly requested:
>  82  *
>  83  *   - the datum will be synchronised to the server if AT_STATX_FORCE_SYNC is
>  84  *     set or if the datum is considered out of date, and
>  85  *
>  86  *   - the field will be filled in and the bit will be set;
>  87  *
>  88  * - otherwise, if not requested, but available in approximate form without any
>  89  *   effort, it will be filled in anyway, and the bit will be set upon return
>  90  *   (it might not be up to date, however, and no attempt will be made to
>  91  *   synchronise the internal state first);
>  92  *
>  93  * - otherwise the field and the bit will be cleared before returning.
>
> The last two paragraphs make it difficult to have a smaller userspace strut statx
> because the kernel will overflow that structure.
>
> I expect the top reserved bit can be used for a one-time expansion, which isn't much
> but more than nothing.
>
> I feel a need to gather our thoughts on this from the glibc side and probably give a
> detailed LPC talk about it to look across the interfaces in question and harmonize
> some kind of design pattern.

that would be great ... let me know if/when you do an LPC talk and
i'll try to come -- it's been a long time since my last LPC -- or at
least give you some potted "i also approve this message" soundbites
:-)

for me these kind of upstream interfaces kind of undermine the "we
should default to having the wrappers in libc" philosophy that i
otherwise broadly agree with. but it's obvious from this thread that
we're not all in complete agreement about what exactly we count as
"compatibility" and how much of it we should/can guarantee (or to
whom). as someone -- iirc fweimer -- said elsewhere on the thread, it
would be good to hear from musl too. even if that probably makes it
even less likely we'll have 100% agreement on anything, it's probably
more useful to present the kernel community with the full range of
perspectives!

> >> I still unsure this is really a good way forward, since it does have some
> >> drawbacks about possible misuse.  This would be first symbol that follow
> >> this new semantic, so I think we should document this properly either
> >> on the wiki that Linux syscall might folow this strategies and how the
> >> wrapper should be added.
> >
> > There isn't any thing special to the initial addition, as long as we
> > don't do emulation.  Strucurally, sched_getattr has the same behavior as
> > recv, for example.  It makes sense to put something into the manual
> > about extending the struct, and I did that.
>
> Agreed.
>
> --
> Cheers,
> Carlos.
>