distexpand for autogenerated upstream distfile resources (was: standardize and simplify GitHub submodule handling in ports?)
On Mon, Aug 07, 2023 at 09:17:05PM +0100, Stuart Henderson wrote:
[...]
> I think maybe I'd prefer to have some variable that could be used
> *instead* of the existing GH_* variables rather than in conjunction with
> them (so they can be used for all GH archive ports, rather than have
> them a special case for multi-distfile ports). If that's the standard
> way to do things we can have a sweep of the tree converting other
> ports (or at least the ones that don't use go.port.mk ;)
>
> It would be kind-of helpful if it could support more than just github
> too (gitlab.com, sr.ht, ..). While that could be done with different
> variables (GH_xx, GL_xx, SRHT_xx etc) they're all a similar enough
> layout to each other that making the site part of the variable itself
> rather than the name would be simpler and easier to add more sites
> (plus it covers the case where you have some port using one file from
> github and one from gitlab, etc).
>
> Playing with syntax ideas, maybe something like this would be easy to
> use for pprts not needing a rename -
>
> SOMEVAR+= github vim vim refs/tags/v9.0.1677
> SOMEVAR+= github vim colorschemes 22986fa2a3d2f7229efd4019fcbca411caa6afbb
>
> or with some auto-renaming (and specifying more of the path to avoid the
> extra GH_WRKSRC which I think might not be enough in some cases anyway -
> a port may have several distfiles that need to go into different base
> dirs) -
>
> SOMEVAR+= github fortran-lang fpm refs/tags/v0.7.0
> OTHERVAR+=github toml-f toml-f e49f5523e4ee67db6628618864504448fb8c8939
> vendor/toml-f
> OTHERVAR+=github urbanjost M_CLI2
> 90a1a146e19c8ad37b0469b8cbd04bc28eb67a50 vendor/M_CLI2
>
> (no idea what to use as real names instead of SOMEVAR/OTHERVAR though!)
>
> How does that sort of thing seem to you? (i.e. using the same basic idea as
> you have for submodules, but making it the standard for all gh distfiles)?
I ran with your suggestion and came up with a solution that I've named
distexpand. The idea is to use templates for commonly used,
automatically generated and therefore predictably named, stored, and
packaged dist files. 2 variables take different arguments/parts that
are 'expanded' with the template to working MASTER_SITESn and
DISTFILES.
The current configuration in the ports Makefile is done like this,
after putting distexpand.port.mk into /usr/ports/infrastructure/mk/:
MODULES += distexpand
DISTEXPAND += template account1 project1 id1(commithash/tag)
DISTEXPANDX += template account2 project2 id2(commithash/tag) targetdir
'template' is currently set up for github, gitlab, and sourcehut. You
can use multiple DISTEXPAND and DISTEXPANDX as needed. This will _not_
use up more MASTER_SITESn, as long as the template stays the same.
Regarding the naming, I'm definitely open to discuss other suggestions.
DISTEXPAND is what I've been able to think of that most clearly conveys
the use of the fragments that are expanded to a full address for
fetching the distfile. DISTEXPANDX - the last 'X' is meant to stand for
'extended' as this is the version that relocates the extracted files to
a target dir. I'm slightly partial to consider naming the variables
instead 'DISTEXPAND4' and 'DISTEXPAND5' which would remind the porter
of the number of components for each version.
For the templating, I used %account, %project, %id, %subdir as the
placeholders. Those are substituted later with :S. I'm open to
suggestions if there may be a more established pattern for placeholders
in strings in Makefile context.
This can replace GH_{ACCOUNT,PROJECT,TAGNAME,COMMIT}. Tags are detected
as such, and in that case a DISTNAME will be set to $project-$tag if
not otherwise set. In other scenarios, a DISTNAME or PKGNAME may need
to be set.
A couple of other things to note compared to before:
- GH_WRKSRC is gone without replacement. Its usefulness was
questionable.
- It includes logic that finds the first MASTER_SITESn that isn't
otherwise used, and throws an ERROR if it overruns past
MASTER_SITES9.
- Using tags is now by just proving '0.1.0' or 'v0.11.2' or other
non-commithash string (the heuristic checks for length to determine
if this is a tag or a commit hash).
- It currently uses 2 longer for-loops that are almost identical, but
one for DISTEXPAND, and the other one for DISTEXPANDX. Given the
limitations in Makefiles, I couldn't think of a way to reuse more
code there.
This doesn't need to be in a module, but this way it's easy to plug in
and experiment with.
I'm attaching the distexpand.port.mk, as well as the patch for using it
with neovim as an example. I've tested this with about 3 dozen ports
that use combinations of mostly github sites, but also a gitlab and a
sourcehut dist source [1].
[1] https://thfr.info/tmp/distexpand-ports.txt
Index: Makefile
===
RCS file: /cvs/ports/editors/neovim/Makefile,v
retrieving revision 1.37
diff -u -p -r1.37 Makefile
---
Re: sosetopt(): merge SO_SND* with corresponding SO_RCV* cases
On Wed, Aug 09, 2023 at 12:41:18AM +0300, Vitaliy Makkoveev wrote:
> I think it's better to merge SO_BINDANY cases from both switch blocks.
> This time SO_LINGER case is separated, so there is no reason for
> dedicated switch block.
OK bluhm@
> Index: sys/kern/uipc_socket.c
> ===
> RCS file: /cvs/src/sys/kern/uipc_socket.c,v
> retrieving revision 1.307
> diff -u -p -r1.307 uipc_socket.c
> --- sys/kern/uipc_socket.c3 Aug 2023 09:49:08 - 1.307
> +++ sys/kern/uipc_socket.c8 Aug 2023 21:35:50 -
> @@ -1800,13 +1800,6 @@ sosetopt(struct socket *so, int level, i
> error = ENOPROTOOPT;
> } else {
> switch (optname) {
> - case SO_BINDANY:
> - if ((error = suser(curproc)) != 0) /* XXX */
> - return (error);
> - break;
> - }
> -
> - switch (optname) {
>
> case SO_LINGER:
> if (m == NULL || m->m_len != sizeof (struct linger) ||
> @@ -1824,6 +1817,9 @@ sosetopt(struct socket *so, int level, i
>
> break;
Can you add a newline here? All other cases have one.
> case SO_BINDANY:
> + if ((error = suser(curproc)) != 0) /* XXX */
> + return (error);
> + /* FALLTHROUGH */
> case SO_DEBUG:
> case SO_KEEPALIVE:
> case SO_USELOOPBACK:
Re: sosetopt(): merge SO_SND* with corresponding SO_RCV* cases
On Tue, Aug 08, 2023 at 10:40:46PM +0200, Alexander Bluhm wrote:
> On Fri, Aug 04, 2023 at 12:38:23AM +0300, Vitaliy Makkoveev wrote:
> > @@ -1856,6 +1856,9 @@ sosetopt(struct socket *so, int level, i
> > case SO_SNDLOWAT:
> > case SO_RCVLOWAT:
> > {
> > + struct sockbuf *sb = (optname == SO_SNDBUF ||
> > + optname == SO_SNDLOWAT ?
> > + >so_snd : >so_rcv);
> > @@ -1910,6 +1896,8 @@ sosetopt(struct socket *so, int level, i
> > case SO_SNDTIMEO:
> > case SO_RCVTIMEO:
> > {
> > + struct sockbuf *sb = (optname == SO_SNDTIMEO ?
> > + >so_snd : >so_rcv);
>
> Would it be nicer to set sb in the switch (optname) at the begining?
>
> struct sockbuf *sb = NULL;
>
> switch (optname) {
> case SO_SNDBUF:
> case SO_SNDLOWAT:
> case SO_SNDTIMEO:
> sb = >so_snd;
> break;
> case SO_RCVBUF:
> case SO_RCVLOWAT:
> case SO_RCVTIMEO:
> sb = >so_rcv;
> break;
> case SO_BINDANY:
> ...
> }
>
> Anyway, OK bluhm@
>
I think it's better to merge SO_BINDANY cases from both switch blocks.
This time SO_LINGER case is separated, so there is no reason for
dedicated switch block.
Index: sys/kern/uipc_socket.c
===
RCS file: /cvs/src/sys/kern/uipc_socket.c,v
retrieving revision 1.307
diff -u -p -r1.307 uipc_socket.c
--- sys/kern/uipc_socket.c 3 Aug 2023 09:49:08 - 1.307
+++ sys/kern/uipc_socket.c 8 Aug 2023 21:35:50 -
@@ -1800,13 +1800,6 @@ sosetopt(struct socket *so, int level, i
error = ENOPROTOOPT;
} else {
switch (optname) {
- case SO_BINDANY:
- if ((error = suser(curproc)) != 0) /* XXX */
- return (error);
- break;
- }
-
- switch (optname) {
case SO_LINGER:
if (m == NULL || m->m_len != sizeof (struct linger) ||
@@ -1824,6 +1817,9 @@ sosetopt(struct socket *so, int level, i
break;
case SO_BINDANY:
+ if ((error = suser(curproc)) != 0) /* XXX */
+ return (error);
+ /* FALLTHROUGH */
case SO_DEBUG:
case SO_KEEPALIVE:
case SO_USELOOPBACK:
Re: sosetopt(): merge SO_SND* with corresponding SO_RCV* cases
On Fri, Aug 04, 2023 at 12:38:23AM +0300, Vitaliy Makkoveev wrote:
> @@ -1856,6 +1856,9 @@ sosetopt(struct socket *so, int level, i
> case SO_SNDLOWAT:
> case SO_RCVLOWAT:
> {
> + struct sockbuf *sb = (optname == SO_SNDBUF ||
> + optname == SO_SNDLOWAT ?
> + >so_snd : >so_rcv);
> @@ -1910,6 +1896,8 @@ sosetopt(struct socket *so, int level, i
> case SO_SNDTIMEO:
> case SO_RCVTIMEO:
> {
> + struct sockbuf *sb = (optname == SO_SNDTIMEO ?
> + >so_snd : >so_rcv);
Would it be nicer to set sb in the switch (optname) at the begining?
struct sockbuf *sb = NULL;
switch (optname) {
case SO_SNDBUF:
case SO_SNDLOWAT:
case SO_SNDTIMEO:
sb = >so_snd;
break;
case SO_RCVBUF:
case SO_RCVLOWAT:
case SO_RCVTIMEO:
sb = >so_rcv;
break;
case SO_BINDANY:
...
}
Anyway, OK bluhm@
Re: pf(4) may cause relayd(8) to abort
On Tue, Aug 01, 2023 at 01:50:52AM +0200, Alexandr Nedvedicky wrote:
> OK to commit?
OK bluhm@
> 8<---8<---8<--8<
> diff --git a/sys/net/pf_table.c b/sys/net/pf_table.c
> index 6f23a6f795d..c862c804f84 100644
> --- a/sys/net/pf_table.c
> +++ b/sys/net/pf_table.c
> @@ -1565,8 +1565,10 @@ pfr_add_tables(struct pfr_table *tbl, int size, int
> *nadd, int flags)
> xadd++;
> } else if (!(flags & PFR_FLAG_DUMMY) &&
> !(p->pfrkt_flags & PFR_TFLAG_ACTIVE)) {
> - p->pfrkt_nflags = (p->pfrkt_flags &
> - ~PFR_TFLAG_USRMASK) | PFR_TFLAG_ACTIVE;
> + p->pfrkt_nflags =
> + (p->pfrkt_flags & ~PFR_TFLAG_USRMASK) |
> + (n->pfrkt_flags & PFR_TFLAG_USRMASK) |
> + PFR_TFLAG_ACTIVE;
> SLIST_INSERT_HEAD(, p, pfrkt_workq);
> }
> }
> diff --git a/regress/sys/net/pf_table/Makefile
> b/regress/sys/net/pf_table/Makefile
> index a71f0190c73..8911e8a1d35 100644
> --- a/regress/sys/net/pf_table/Makefile
> +++ b/regress/sys/net/pf_table/Makefile
> @@ -1,15 +1,26 @@
> #$OpenBSD: Makefile,v 1.3 2017/07/07 23:15:27 bluhm Exp $
>
> -REGRESS_TARGETS= hit miss cleanup
> -CLEANFILES= stamp-*
> +REGRESS_TARGETS= hit miss cleanup flags
> +CLEANFILES= stamp-* \
> + pf-reftab.conf \
> + pf-instance.conf\
> + table-ref.conf \
> + table-pgone.out \
> + table-persist.out \
> + table-ref.out \
> + table-refgone.out
> +
>
> stamp-setup:
> + ${SUDO} pfctl -a regress/ttest -Fa
> ${SUDO} pfctl -qt __regress_tbl -T add -f ${.CURDIR}/table.in
> date >$@
>
> cleanup:
> rm -f stamp-setup
> ${SUDO} pfctl -qt __regress_tbl -T kill
> + ${SUDO} pfctl -q -a regress/ttest -Fr
> + ${SUDO} pfctl -q -a regress/ttest -qt instance -T kill
>
> hit: stamp-setup
> for i in `cat ${.CURDIR}/table.hit`; do \
> @@ -27,6 +38,77 @@ miss: stamp-setup
> done; \
> exit 0
>
> -.PHONY: hit miss
> +#
> +# tables and are both referenced by rule only
> +#
> +pf-instab.conf:
> + @echo 'table { 192.168.1.0/24 }' > $@
> + @echo 'pass in from to ' >> $@
> +
> +#
> +# table is active and referred by rule, table
> +# is referenced only.
> +pf-reftab.conf:
> + @echo 'pass in from to ' > $@
> +
> +#
> +# check persistent flag (p) is gone from table after
> +# we load pf-instab.conf. Deals with case when persistent table
> +# exists before pf-instab.conf gets loaded.
> +#
> +table-pgone.out:
> + @echo '--a-r-- instanceregress/ttest' > $@
> + @echo 'r-- reference regress/ttest' >> $@
> +
> +#
> +# verify table got persistent flag after we
> +# run 'pfctl -t instance -T add ...'
> +#
> +table-persist.out:
> + @echo '-pa-r-- instanceregress/ttest' > $@
> + @echo 'r-- reference regress/ttest' >> $@
> +
> +#
> +# verify tables and are created on behalf of
> +# reference by rule after pf-reftab.conf got loaded.
> +#
> +table-ref.out:
> + @echo 'r-- instanceregress/ttest' > $@
> + @echo 'r-- reference regress/ttest' >> $@
> +
> +#
> +# verify reference to table (persistent) is gone
> +# after rules got flushed
> +#
> +table-refgone.out:
> + @echo '-pa instanceregress/ttest' > $@
> +
> +flags: pf-instab.conf pf-reftab.conf table-pgone.out table-persist.out \
> +table-ref.out table-refgone.out
> + @echo 'loading pf-reftab,conf (tables referenced by rules only)'
> + @cat pf-reftab.conf
> + ${SUDO} pfctl -a regress/ttest -f pf-reftab.conf
> + @echo 'tables and should both have r--'
> + ${SUDO} pfctl -a regress/ttest -sT -vg | diff table-ref.out -
> + @echo 'creating table on command line, flags should be:'
> + @cat table-persist.out
> + ${SUDO} pfctl -a regress/ttest -t instance -T add 192.168.1.0/24
> + ${SUDO} pfctl -a regress/ttest -sT -vg | diff table-persist.out -
> + @echo 'flushing rules'
> + ${SUDO} pfctl -a regress/ttest -Fr
> + @echo 'table should be gone, table should stay'
> + ${SUDO} pfctl -a regress/ttest -sT -vg | diff table-refgone.out -
> + @echo 'loading pf-instab.conf'
> + @cat pf-instab.conf
> + ${SUDO} pfctl -a regress/ttest -f pf-instab.conf
> + @echo 'table loses -p- flag:'
> + @cat table-pgone.out
> + ${SUDO} pfctl -a regress/ttest -sT -vg | diff table-pgone.out -
> + @echo 'flusing rules, both tables should be gone'
> + ${SUDO} pfctl -a regress/ttest -Fr
> + @echo 'anchor regress/ttest must be gone'
> + ${SUDO} pfctl -a regress/ttest -sr 2>&1 | grep 'pfctl: Anchor does not
> exist'
Re: agtimer(4/arm64): simplify agtimer_delay()
> From: Dale Rahn
> Date: Tue, 8 Aug 2023 12:36:45 -0400
>
> Switching the computation of cycles/delaycnt to a proper 64 value math
> instead of the '32 bit safe' complex math is likely a good idea.
> However I am not completely convinced that switching to 'yield' (current
> CPU_BUSY_CYCLE implementation) for every loop of a 'short wait' in the wait
> loop makes sense. In a hypervisor environment, this could cause a very
> short wait between register writes to become very long, In a non-hypervisor
> environment there is essentially no improvement because the yield doesn't
> really have any benefits on non-hypervisor.
Dale, I think you're confused here. There is no arcitectural way to
trap a YIELD instruction. The docs explicitly state that SMT and SMP
systems. I suspect that this instruction was primarily introduced for
SMT systems that never materialized; I completely believe you that on
current hardware it does nothing.
Even without a YIELD instruction a hypervisor might interrupt us and
schedule a different vCPU onto the core. And on real hardware
external interrupts may happen. So you really can't count on delay(9)
being accurate.
Linux does use YIELD in it delay loop.
> To my current understanding, there is no useful 'wait short period' on arm
> cores.
There is WFET (and WFIT), but that is Armv8.7 material, so not
availble on actual hardware that we run on. Linux uses that
instruction in its delay loop when available.
On machines with a working Generic Timer event stream, Linux uses WFE
if the delay is long enough.
> On Mon, Aug 7, 2023 at 10:32 PM Scott Cheloha
> wrote:
>
> > The agtimer(4/arm64) delay(9) implementation is quite complicated.
> > This patch simplifies it.
> >
> > Am I missing something here? There's no reason to implement the
> > busy-loop like this, right?
> >
> > ok?
> >
> > Index: agtimer.c
> > ===
> > RCS file: /cvs/src/sys/arch/arm64/dev/agtimer.c,v
> > retrieving revision 1.23
> > diff -u -p -r1.23 agtimer.c
> > --- agtimer.c 25 Jul 2023 18:16:19 - 1.23
> > +++ agtimer.c 8 Aug 2023 02:24:57 -
> > @@ -323,32 +323,12 @@ agtimer_cpu_initclocks(void)
> > void
> > agtimer_delay(u_int usecs)
> > {
> > - uint64_tclock, oclock, delta, delaycnt;
> > - uint64_tcsec, usec;
> > - volatile intj;
> > + uint64_t cycles, start;
> >
> > - if (usecs > (0x8000 / agtimer_frequency)) {
> > - csec = usecs / 1;
> > - usec = usecs % 1;
> > -
> > - delaycnt = (agtimer_frequency / 100) * csec +
> > - (agtimer_frequency / 100) * usec / 1;
> > - } else {
> > - delaycnt = agtimer_frequency * usecs / 100;
> > - }
> > - if (delaycnt <= 1)
> > - for (j = 100; j > 0; j--)
> > - ;
> > -
> > - oclock = agtimer_readcnt64();
> > - while (1) {
> > - for (j = 100; j > 0; j--)
> > - ;
> > - clock = agtimer_readcnt64();
> > - delta = clock - oclock;
> > - if (delta > delaycnt)
> > - break;
> > - }
> > + start = agtimer_readcnt64();
> > + cycles = (uint64_t)usecs * agtimer_frequency / 100;
> > + while (agtimer_readcnt64() - start < cycles)
> > + CPU_BUSY_CYCLE();
> > }
> >
> > void
> >
> >
>
Re: EVFILT_TIMER add support for different timer precisions NOTE_{,U,N,M}SECONDS
On Tue, Aug 08, 2023 at 10:40:06AM -0500, Scott Cheloha wrote:
> On Sat, Aug 05, 2023 at 01:33:05AM -0400, A Tammy wrote:
> >
> > On 8/5/23 00:49, Scott Cheloha wrote:
> > > On Sat, Aug 05, 2023 at 12:17:48AM -0400, aisha wrote:
> > >> On 22/09/10 01:53PM, Visa Hankala wrote:
> > >>> On Wed, Aug 31, 2022 at 04:48:37PM -0400, aisha wrote:
> > I've added a patch which adds support for NOTE_{,U,M,N}SECONDS for
> > EVFILT_TIMER in the kqueue interface.
> > >>> It sort of makes sense to add an option to specify timeouts in
> > >>> sub-millisecond precision. It feels complete overengineering to add
> > >>> multiple time units on the level of the kernel interface. However,
> > >>> it looks that FreeBSD and NetBSD have already done this following
> > >>> macOS' lead...
> > >>>
> > I've also added the NOTE_ABSTIME but haven't done any actual
> > implementation
> > there as I am not sure how the `data` field should be interpreted (is
> > it
> > absolute time in seconds since epoch?).
> > >>> I think FreeBSD and NetBSD take NOTE_ABSTIME as time since the epoch.
> > >>>
> > >>> Below is a revised patch that takes into account some corner cases.
> > >>> It tries to be API-compatible with FreeBSD and NetBSD. I have adjusted
> > >>> the NOTE_{,M,U,N}SECONDS flags so that they are enum-like.
> > >>>
> > >>> The manual page bits are from NetBSD.
> > >>>
> > >>> It is quite late to introduce a feature like this within this release
> > >>> cycle. Until now, the timer code has ignored the fflags field. There
> > >>> might be pieces of software that are careless with struct kevent and
> > >>> that would break as a result of this patch. Programs that are widely
> > >>> used on different BSDs are probably fine already, though.
> > >>
> > >> Sorry, I had forgotten this patch for a long time!!! I've been running
> > >> with this for a while now and it's been working nicely.
> > >
> > > Where is this being used in ports? I think having "one of each" for
> > > seconds, milliseconds, microseconds, and nanoseconds is (as visa
> > > noted) way, way over-the-top.
> >
> > I was using it with a port that I sent out a while ago but never got
> > into tree (was before I joined the project) -
> > https://marc.info/?l=openbsd-ports=165715874509440=2
>
> If nothing in ports is using this I am squeamish about adding it.
> Once we add it, we're stuck maintaining it, warts and all.
>
> If www/workflow were in the tree I could see the upside. Is it in
> ports?
>
> It looks like workflow actually wants timerfd(2) from Linux and is
> simulating timerfd(2) with EVFILT_TIMER and NOTE_NSECONDS:
>
> https://github.com/sogou/workflow/blob/80b3dfbad2264bcd79ba37811c66421490e337d2/src/kernel/poller.c#L227
>
> I think timerfd(2) is the superior interface here. It keeps the POSIX
> interval timer semantics without all the signal delivery baggage. It
> also supports multiple clocks and starting a periodic timeout from an
> absolute starting time.
>
> So, if the goal is "add www/workflow to ports", adding timerfd(2) might
> be the right thing.
I don't think that this is a good move. Adding timerfd(2) will result in
the need to add all those magic fd interfaces linux invents on a weekly
basis. I would not go down that rabbit-hole unless there is realy realy no
alternative.
--
:wq Claudio
Re: agtimer(4/arm64): simplify agtimer_delay()
Switching the computation of cycles/delaycnt to a proper 64 value math
instead of the '32 bit safe' complex math is likely a good idea.
However I am not completely convinced that switching to 'yield' (current
CPU_BUSY_CYCLE implementation) for every loop of a 'short wait' in the wait
loop makes sense. In a hypervisor environment, this could cause a very
short wait between register writes to become very long, In a non-hypervisor
environment there is essentially no improvement because the yield doesn't
really have any benefits on non-hypervisor.
To my current understanding, there is no useful 'wait short period' on arm
cores.
On Mon, Aug 7, 2023 at 10:32 PM Scott Cheloha
wrote:
> The agtimer(4/arm64) delay(9) implementation is quite complicated.
> This patch simplifies it.
>
> Am I missing something here? There's no reason to implement the
> busy-loop like this, right?
>
> ok?
>
> Index: agtimer.c
> ===
> RCS file: /cvs/src/sys/arch/arm64/dev/agtimer.c,v
> retrieving revision 1.23
> diff -u -p -r1.23 agtimer.c
> --- agtimer.c 25 Jul 2023 18:16:19 - 1.23
> +++ agtimer.c 8 Aug 2023 02:24:57 -
> @@ -323,32 +323,12 @@ agtimer_cpu_initclocks(void)
> void
> agtimer_delay(u_int usecs)
> {
> - uint64_tclock, oclock, delta, delaycnt;
> - uint64_tcsec, usec;
> - volatile intj;
> + uint64_t cycles, start;
>
> - if (usecs > (0x8000 / agtimer_frequency)) {
> - csec = usecs / 1;
> - usec = usecs % 1;
> -
> - delaycnt = (agtimer_frequency / 100) * csec +
> - (agtimer_frequency / 100) * usec / 1;
> - } else {
> - delaycnt = agtimer_frequency * usecs / 100;
> - }
> - if (delaycnt <= 1)
> - for (j = 100; j > 0; j--)
> - ;
> -
> - oclock = agtimer_readcnt64();
> - while (1) {
> - for (j = 100; j > 0; j--)
> - ;
> - clock = agtimer_readcnt64();
> - delta = clock - oclock;
> - if (delta > delaycnt)
> - break;
> - }
> + start = agtimer_readcnt64();
> + cycles = (uint64_t)usecs * agtimer_frequency / 100;
> + while (agtimer_readcnt64() - start < cycles)
> + CPU_BUSY_CYCLE();
> }
>
> void
>
>
Re: EVFILT_TIMER add support for different timer precisions NOTE_{,U,N,M}SECONDS
On Sat, Aug 05, 2023 at 01:33:05AM -0400, A Tammy wrote:
>
> On 8/5/23 00:49, Scott Cheloha wrote:
> > On Sat, Aug 05, 2023 at 12:17:48AM -0400, aisha wrote:
> >> On 22/09/10 01:53PM, Visa Hankala wrote:
> >>> On Wed, Aug 31, 2022 at 04:48:37PM -0400, aisha wrote:
> I've added a patch which adds support for NOTE_{,U,M,N}SECONDS for
> EVFILT_TIMER in the kqueue interface.
> >>> It sort of makes sense to add an option to specify timeouts in
> >>> sub-millisecond precision. It feels complete overengineering to add
> >>> multiple time units on the level of the kernel interface. However,
> >>> it looks that FreeBSD and NetBSD have already done this following
> >>> macOS' lead...
> >>>
> I've also added the NOTE_ABSTIME but haven't done any actual
> implementation
> there as I am not sure how the `data` field should be interpreted (is it
> absolute time in seconds since epoch?).
> >>> I think FreeBSD and NetBSD take NOTE_ABSTIME as time since the epoch.
> >>>
> >>> Below is a revised patch that takes into account some corner cases.
> >>> It tries to be API-compatible with FreeBSD and NetBSD. I have adjusted
> >>> the NOTE_{,M,U,N}SECONDS flags so that they are enum-like.
> >>>
> >>> The manual page bits are from NetBSD.
> >>>
> >>> It is quite late to introduce a feature like this within this release
> >>> cycle. Until now, the timer code has ignored the fflags field. There
> >>> might be pieces of software that are careless with struct kevent and
> >>> that would break as a result of this patch. Programs that are widely
> >>> used on different BSDs are probably fine already, though.
> >>
> >> Sorry, I had forgotten this patch for a long time!!! I've been running
> >> with this for a while now and it's been working nicely.
> >
> > Where is this being used in ports? I think having "one of each" for
> > seconds, milliseconds, microseconds, and nanoseconds is (as visa
> > noted) way, way over-the-top.
>
> I was using it with a port that I sent out a while ago but never got
> into tree (was before I joined the project) -
> https://marc.info/?l=openbsd-ports=165715874509440=2
If nothing in ports is using this I am squeamish about adding it.
Once we add it, we're stuck maintaining it, warts and all.
If www/workflow were in the tree I could see the upside. Is it in
ports?
It looks like workflow actually wants timerfd(2) from Linux and is
simulating timerfd(2) with EVFILT_TIMER and NOTE_NSECONDS:
https://github.com/sogou/workflow/blob/80b3dfbad2264bcd79ba37811c66421490e337d2/src/kernel/poller.c#L227
I think timerfd(2) is the superior interface here. It keeps the POSIX
interval timer semantics without all the signal delivery baggage. It
also supports multiple clocks and starting a periodic timeout from an
absolute starting time.
So, if the goal is "add www/workflow to ports", adding timerfd(2) might
be the right thing.
> I also agree with it being over the top but that's the way it is in
> net/freebsd, I'm also fine with breaking compatibility and only keeping
> nano, no preferences either way.
Well, if we're going to add it (if), we should add all of it. The
vast majority of the code is not conversion code: if we add support
for NOTE_NSECONDS, adding support for the other units is trivial, and
there is value in being fully compatible with other implementations.
> > The original EVFILT_TIMER supported only milliseconds, yes. Given
> > that it debuted in the late 90s, I think that was a bad choice. But
> > when milliseconds were insufficiently precise, the obvious thing would
> > be to add support for nanoseconds... and then stop.
> >
> > The decision to use the UTC clock with no option to select a different
> > clockid_t for NOTE_ABSTIME is also unfortunate.
>
> Yes, furthermore this was very unclear as I couldn't find this in the
> man pages for either of net/freebsd.
>
> > Grumble.
> >
> >> I had an unrelated question inlined.
> >>
> >> [...]
> >>> static void
> >>> -filt_timer_timeout_add(struct knote *kn)
> >>> +filt_timeradd(struct knote *kn, struct timespec *ts)
> >>> {
> >>> - struct timeval tv;
> >>> + struct timespec expiry, now;
> >>> struct timeout *to = kn->kn_hook;
> >>> int tticks;
> >>>
> >>> - tv.tv_sec = kn->kn_sdata / 1000;
> >>> - tv.tv_usec = (kn->kn_sdata % 1000) * 1000;
> >>> - tticks = tvtohz();
> >>> - /* Remove extra tick from tvtohz() if timeout has fired before. */
> >>> + if (kn->kn_sfflags & NOTE_ABSTIME) {
> >>> + nanotime();
> >>> + if (timespeccmp(ts, , >)) {
> >>> + timespecsub(ts, , );
> >>> + /* XXX timeout_at_ts */
> >>> + timeout_add(to, tstohz());
> > visa:
> >
> > we should use timeout_abs_ts() here. I need to adjust it, though.
> >
> >>> + } else {
> >>> + /* Expire immediately. */
> >>> + filt_timerexpire(kn);
> >>> + }
> >>> + return;
> >>> + }
> >>> +
> >>> + tticks = tstohz(ts);
> >>> + /* Remove
Re: PATCH: a bit of introspection in make
Actually, as far as bsd.port.mk, it doesn't need to
move too much stuff around thanks to make's lazyness.
Note that having a list of defined MASTER_SITES variables simplifies
the check.
I've also added a check for the right MASTER_SITES to be defined,
since currently we do not error out until actually using it, which
means that fiddling around with MASTER_SITES before committing may
often go unnoticed.
(That final part is meant to go in sooner rather than later)
Index: bsd.port.mk
===
RCS file: /cvs/ports/infrastructure/mk/bsd.port.mk,v
retrieving revision 1.1592
diff -u -p -r1.1592 bsd.port.mk
--- bsd.port.mk 13 Jun 2023 10:28:40 - 1.1592
+++ bsd.port.mk 8 Aug 2023 10:59:38 -
@@ -118,9 +118,8 @@ _ALL_VARIABLES_PER_ARCH =
# consumers of (dump-vars) include sqlports generation and dpb
# dpb doesn't need everything, those are speed optimizations
.if ${DPB:L:Mfetch} || ${DPB:L:Mall}
-_ALL_VARIABLES += DISTFILES PATCHFILES SUPDISTFILES DIST_SUBDIR MASTER_SITES \
- MASTER_SITES0 MASTER_SITES1 MASTER_SITES2 MASTER_SITES3 MASTER_SITES4 \
- MASTER_SITES5 MASTER_SITES6 MASTER_SITES7 MASTER_SITES8 MASTER_SITES9 \
+_ALL_VARIABLES += DISTFILES PATCHFILES SUPDISTFILES DIST_SUBDIR \
+ ${_ALL_MASTER_SITES} \
CHECKSUM_FILE FETCH_MANUALLY MISSING_FILES PERMIT_DISTFILES
.endif
.if ${DPB:L:Mtest} || ${DPB:L:Mall}
@@ -1280,19 +1280,15 @@ MASTER_SITES ?=
# sites for distfiles, add them to MASTER_SITE_BACKUP
_warn_checksum = :
-.if !empty(MASTER_SITES:M*[^/])
-_warn_checksum += ;echo ">>> MASTER_SITES not ending in /:
${MASTER_SITES:M*[^/]}"
-.endif
-.for _I in 0 1 2 3 4 5 6 7 8 9
-. if defined(MASTER_SITES${_I})
-.if !empty(MASTER_SITES${_I}:M*[^/])
-_warn_checksum += ;echo ">>> MASTER_SITES${_I} not ending in /:
${MASTER_SITES${_I}:M*[^/]}"
-.endif
+_ALL_MASTER_SITES = ${.VARIABLES:MMASTER_SITES*:NMASTER_SITES_*}
+
+.for _S in ${_ALL_MASTER_SITES}
+. if !empty(${_S}:M*[^/])
+_warn_checksum += ;echo ">>> ${_S} not ending in /: ${${_S}:M*[^/]}"
. endif
.endfor
-
EXTRACT_SUFX ?= .tar.gz
.if !empty(GH_COMMIT)
@@ -1322,6 +1318,9 @@ _FILES=
. if !empty($v)
.for e in ${$v}
. for f m u in ${e:C/:[0-9]$//:C/^(.*)\{.*\}(.*)$/\1\2/}
MASTER_SITES${e:M*\:[0-9]:C/^.*:([0-9])$/\1/}
${e:C/:[0-9]$//:C/^.*\{(.*)\}(.*)$/\1\2/}
+.if !defined($m)
+ERRORS += "Fatal: $m is not defined but referenced by $e in $v"
+.endif
.if empty(_FILES:M$f)
_FILES += $f
. if empty(DIST_SUBDIR)
Re: PATCH: a bit of introspection in make
Here's a revised diff (reordered plus actual use of the boolean)
plus concrete example use for bsd.port.mk (disregarding the fact
_ALL_VARIABLES would have to move *after* all MASTER_SITES have been defined.
Index: var.c
===
RCS file: /cvs/src/usr.bin/make/var.c,v
retrieving revision 1.104
diff -u -p -r1.104 var.c
--- var.c 9 Jun 2022 13:13:14 - 1.104
+++ var.c 8 Aug 2023 10:48:05 -
@@ -104,6 +104,8 @@ static char varNoError[] = "";
bool errorIsOkay;
static boolcheckEnvFirst; /* true if environment should be searched for
* variables before the global context */
+ /* do we need to recompute varname_list */
+static boolvarname_list_changed = true;
void
Var_setCheckEnvFirst(bool yes)
@@ -222,6 +224,7 @@ typedef struct Var_ {
#define VAR_FROM_ENV 8 /* Special source: environment */
#define VAR_SEEN_ENV 16 /* No need to go look up environment again */
#define VAR_IS_SHELL 32 /* Magic behavior */
+#define VAR_IS_NAMES 1024/* Very expensive, only defined when needed */
/* XXX there are also some flag values which are part of the visible API
* and thus defined inside var.h, don't forget to look there if you want
* to define some new flags !
@@ -231,6 +234,8 @@ typedef struct Var_ {
char name[1]; /* the variable's name */
} Var;
+/* for GNU make compatibility */
+#define VARNAME_LIST ".VARIABLES"
static struct ohash_info var_info = {
offsetof(Var, name),
@@ -245,10 +250,11 @@ static void fill_from_env(Var *);
static Var *create_var(const char *, const char *);
static void var_set_initial_value(Var *, const char *);
static void var_set_value(Var *, const char *);
-#define var_get_value(v) ((v)->flags & VAR_EXEC_LATER ? \
- var_exec_cmd(v) : \
- Buf_Retrieve(&((v)->val)))
-static char *var_exec_cmd(Var *);
+static char *var_get_value(Var *);
+static void var_exec_cmd(Var *);
+static void varname_list_retrieve(Var *);
+
+
static void var_append_value(Var *, const char *);
static void poison_check(Var *);
static void var_set_append(const char *, const char *, const char *, int,
bool);
@@ -423,6 +429,7 @@ var_set_initial_value(Var *v, const char
len = strlen(val);
Buf_Init(&(v->val), len+1);
Buf_AddChars(&(v->val), len, val);
+ varname_list_changed = true;
}
/* Normal version of var_set_value(), to be called after variable is fully
@@ -440,6 +447,16 @@ var_set_value(Var *v, const char *val)
}
}
+static char *
+var_get_value(Var *v)
+{
+ if (v->flags & VAR_IS_NAMES)
+ varname_list_retrieve(v);
+ else if (v->flags & VAR_EXEC_LATER)
+ var_exec_cmd(v);
+ return Buf_Retrieve(&(v->val));
+}
+
/* Add to a variable, insert a separating space if the variable was already
* defined.
*/
@@ -628,6 +645,7 @@ Var_Deletei(const char *name, const char
ohash_remove(_variables, slot);
delete_var(v);
+ varname_list_changed = true;
}
/* Set or add a global variable, either to VAR_CMD or VAR_GLOBAL.
@@ -687,7 +705,7 @@ Var_Appendi_with_ctxt(const char *name,
var_set_append(name, ename, val, ctxt, true);
}
-static char *
+static void
var_exec_cmd(Var *v)
{
char *arg = Buf_Retrieve(&(v->val));
@@ -699,7 +717,30 @@ var_exec_cmd(Var *v)
var_set_value(v, res1);
free(res1);
v->flags &= ~VAR_EXEC_LATER;
- return Buf_Retrieve(&(v->val));
+}
+
+static void
+varname_list_retrieve(Var *v)
+{
+ unsigned int i;
+ void *e;
+ bool first = true;
+
+ if (!varname_list_changed)
+ return;
+ for (e = ohash_first(_variables, ); e != NULL;
+ e = ohash_next(_variables, )) {
+ Var *v2 = e;
+ if (v2->flags & VAR_DUMMY)
+ continue;
+
+ if (first)
+ var_set_value(v, v2->name);
+ else
+ var_append_value(v, v2->name);
+ first = false;
+ }
+ varname_list_changed = false;
}
/* XXX different semantics for Var_Valuei() and Var_Definedi():
@@ -1339,6 +1380,19 @@ set_magic_shell_variable()
v->flags = VAR_IS_SHELL | VAR_SEEN_ENV;
}
+static void
+set_magic_name_list_variable()
+{
+ const char *name = VARNAME_LIST;
+ const char *ename = NULL;
+ uint32_t k;
+ Var *v;
+
+ k = ohash_interval(name, );
+ v = find_global_var_without_env(name, ename, k);
+ var_set_initial_value(v, "");
+ v->flags = VAR_IS_NAMES;
+}
/*
* Var_Init
* Initialize the module
@@ -1348,11 +1402,10 @@ Var_Init(void)
{
ohash_init(_variables, 10, _info);
set_magic_shell_variable();
-
+ set_magic_name_list_variable();
errorIsOkay = true;
