Date: Thu, 16 Mar 2023 09:41:17 -0600 From: Warner Losh <imp@bsdimp.com> To: Justin Hibbits <jhibbits@freebsd.org> Cc: "freebsd-arch@freebsd.org" <freebsd-arch@freebsd.org> Subject: Re: Blocks runtime in the kernel Message-ID: <CANCZdfqohTcujkDx=rizAXYAKXaDDw09g_hK6WSZ1JGKvvGOgA@mail.gmail.com> In-Reply-To: <20230316112222.31b1620e@gonegalt.net> References: <20230316100611.4892008c@gonegalt.net> <CANCZdfrqSc5cuU=Pbo_fGKmbTeeoTYUaYPqZUahRar1XkbJ6=A@mail.gmail.com> <20230316112222.31b1620e@gonegalt.net>
next in thread | previous in thread | raw e-mail | index | archive | help
--000000000000489b0605f7064b0f
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
On Thu, Mar 16, 2023 at 9:22=E2=80=AFAM Justin Hibbits <jhibbits@freebsd.or=
g> wrote:
> On Thu, 16 Mar 2023 09:04:29 -0600
> Warner Losh <imp@bsdimp.com> wrote:
>
> > On Thu, Mar 16, 2023, 8:06 AM Justin Hibbits <jhibbits@freebsd.org>
> > wrote:
> >
> > > Most probably know I've been working on the IfAPI conversion of all
> > > network drivers in order to hide the contents of `struct ifnet`.
> > > I'm pretty much done with the development, and it's all in review.
> > > However, there's one bit that I've thought is very clunky since I
> > > added it, the if_foreach() iterator function, which iterates over
> > > all interfaces in the current VNET, and calls a callback to operate
> > > on each interface. I've noticed that oftentimes I end up with a 2
> > > line callback, which just calls if_foreach_addr_type(), so I end up
> > > with just trivial callback functions, which seems like a waste.
> > >
> > > All that backstory to say, would it be beneficial to anyone else to
> > > add a (very basic) blocks runtime to the kernel for doing things
> > > like this? The rough change to the IfAPI becomes:
> > >
> > > int if_foreach_b(int (^)(if_t));
> > >
> > > __block int foo =3D 0;
> > >
> > > if_foreach_b(^(if_t ifp) {
> > > if (if_getlinkstate(ifp) =3D=3D LINK_STATE_UP)
> > > foo++;
> > > });
> > >
> > > The same could be done for other *_foreach KPIs as well, if this
> > > proves out. I think I could have something working in the next
> > > several days.
> > >
> > > The only technical snag I see with this would be other compilers.
> > > I'm not sure if GCC still supports blocks, it did at one point.
> > >
> > > What do you think?
> > >
> >
> >
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=3D78352
> >
> > Suggests that there were issues upstreaming the apple code. So
> > there's that. The gcc12 port I have can't cope with the sample blocks
> > code I found on Wikipedia:
> > /* blocks-test.c */
> > #include <stdio.h>
> > #include <Block.h>
> > /* Type of block taking nothing returning an int */
> > typedef int (^IntBlock)();
> >
> > IntBlock MakeCounter(int start, int increment) {
> > __block int i =3D start;
> >
> > return Block_copy( ^(void) {
> > int ret =3D i;
> > i +=3D increment;
> > return ret;
> > });
> >
> > }
> >
> > int main(void) {
> > IntBlock mycounter =3D MakeCounter(5, 2);
> > printf("First call: %d\n", mycounter());
> > printf("Second call: %d\n", mycounter());
> > printf("Third call: %d\n", mycounter());
> >
> > /* because it was copied, it must also be released */
> > Block_release(mycounter);
> >
> > return 0;
> > }
> >
> > Our current clang is OK:
> > % clang -fblocks a.c -o a -lBlocksRuntime
> > %
> >
> > But there's no current users of __block in the kernel. There's no
> > kernel-specific Block.h file,
> > there's no references to BlockRuntime anywhere in the kernel tree and
> > the code in
> > contrib/llvm-project/compiler-rt/lib/BlocksRuntime is completely
> > userland specific. There
> > is no kernel support that I could see, since we don't have a
> > libkern/OSAtomic.h. I'm happy
> > to be corrected on this though: I've never tried to use blocks in the
> > kernel and this is grep
> > level confidence.
> >
> > Clang also doesn't enable blocks unless you pass it -fblock, so you'd
> > need to change a fair
> > portion of the kernel build system to enable that.
> >
> > So I'm thinking regardless of whether or not the project should do
> > this, you'll have a fair amount
> > of choppy waves ahead of you before you could get to the point of
> > starting the ifnet work.
> >
> > Warner
>
> Hi Warner,
>
> I did a very very simple test to see what is required link-wise for
> blocks in kernel. This was done by changing
> https://reviews.freebsd.org/D38962 to use a block instead of a callback
> for the "bootpc_init_count_if_cb". I didn't include Block.h or
> anything, and simply added "-fblocks" to kern.mk. The result is it
> compiles fine, then fails to link (expected) with the following missing
> symbols:
>
As a basic test, that's fine. But I'm not sure we want to globally add
-fblocks
to the kernel. I don't know if that changes anything else. People will want
to know if there's global performance or size impact from doing this and
whether or not the compiler inserts other code because blocks are possible.
_Block_object_dispose
> _Block_object_assign
> _NSConcreteStackBlock
>
> Reading through
> contrib/llvm-project/compiler-rt/lib/BlocksRuntime/runtime.c these
> missing symbols look straightforward to implement for the basic case.
> I'm not thinking of working within the Clang runtime.c, I'm thinking of
> reimplementing the needed functions for this constrained use case (no
> need for GC, etc).
>
> This testing was only marginally more than you did, so I'm probably
> missing some things as well for more complex use cases.
>
I was worried about two things when I looked at the code: reference countin=
g
(which I think is kinda required, even though objc uses it for GC) and
memory
allocation / handling. The former is well understood, and we can adapt
things
(though knowing which subset is required here might be tricky, there's a lo=
t
of flags). The latter, though, would limit the use of these APIs to
situations
where you can call malloc/free M_WAIT, or you'd need to deal with malloc
failures better than runtime.c does.
So while the number of routines is small, I think they are the tip of the
iceburg
and may be more work than you're suggesting.
> I'm guessing from GCC's issues that this is a nonstarter anyway?
>
In the past we've said that it's OK to use clang specific code to get bette=
r
performance, but that depending on it entirely would require a careful
discussion. gcc is produces code that's easier to debug than clang (though
gcc12 build is currently still broken), and that's not nothing and has been
useful for me in the past. jhb can likely speak to other benefits for gcc12
since he did the last round of updates.
So given the difficulties on multiple fronts, I'm not sure it's a great
idea.
But maybe I'm wrong about how difficult things will be and maybe it would
work out in the end. But selecting the network stack to use what will be
an unproven, or at least immature technology is ambitious. We've had a
mixed bag with that stuff (see epoch and smr for examples).
I'm trying hard not to say a flat out "no," because I know that sometimes
things that look hard like this pay off. But no gcc support does make it
really hard to say yes. I've had my say, and I'll let others say from
here.
Warner
--000000000000489b0605f7064b0f
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
<div dir=3D"ltr"><div dir=3D"ltr"><br></div><br><div class=3D"gmail_quote">=
<div dir=3D"ltr" class=3D"gmail_attr">On Thu, Mar 16, 2023 at 9:22=E2=80=AF=
AM Justin Hibbits <<a href=3D"mailto:jhibbits@freebsd.org">jhibbits@free=
bsd.org</a>> wrote:<br></div><blockquote class=3D"gmail_quote" style=3D"=
margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-lef=
t:1ex">On Thu, 16 Mar 2023 09:04:29 -0600<br>
Warner Losh <<a href=3D"mailto:imp@bsdimp.com" target=3D"_blank">imp@bsd=
imp.com</a>> wrote:<br>
<br>
> On Thu, Mar 16, 2023, 8:06 AM Justin Hibbits <<a href=3D"mailto:jhi=
bbits@freebsd.org" target=3D"_blank">jhibbits@freebsd.org</a>><br>
> wrote:<br>
> <br>
> > Most probably know I've been working on the IfAPI conversion =
of all<br>
> > network drivers in order to hide the contents of `struct ifnet`.<=
br>
> > I'm pretty much done with the development, and it's all i=
n review.<br>
> > However, there's one bit that I've thought is very clunky=
since I<br>
> > added it, the if_foreach() iterator function, which iterates over=
<br>
> > all interfaces in the current VNET, and calls a callback to opera=
te<br>
> > on each interface.=C2=A0 I've noticed that oftentimes I end u=
p with a 2<br>
> > line callback, which just calls if_foreach_addr_type(), so I end =
up<br>
> > with just trivial callback functions, which seems like a waste.<b=
r>
> ><br>
> > All that backstory to say, would it be beneficial to anyone else =
to<br>
> > add a (very basic) blocks runtime to the kernel for doing things<=
br>
> > like this?=C2=A0 The rough change to the IfAPI becomes:<br>
> ><br>
> > int if_foreach_b(int (^)(if_t));<br>
> ><br>
> > __block int foo =3D 0;<br>
> ><br>
> > if_foreach_b(^(if_t ifp) {<br>
> >=C2=A0 =C2=A0if (if_getlinkstate(ifp) =3D=3D LINK_STATE_UP)<br>
> >=C2=A0 =C2=A0 =C2=A0foo++;<br>
> > });<br>
> ><br>
> > The same could be done for other *_foreach KPIs as well, if this<=
br>
> > proves out.=C2=A0 I think I could have something working in the n=
ext<br>
> > several days.<br>
> ><br>
> > The only technical snag I see with this would be other compilers.=
<br>
> > I'm not sure if GCC still supports blocks, it did at one poin=
t.<br>
> ><br>
> > What do you think?<br>
> >=C2=A0 <br>
> <br>
> <br>
> <a href=3D"https://gcc.gnu.org/bugzilla/show_bug.cgi?id=3D78352" rel=
=3D"noreferrer" target=3D"_blank">https://gcc.gnu.org/bugzilla/show_bug.cgi=
?id=3D78352</a><br>
> <br>
> Suggests that there were issues upstreaming the apple code. So<br>
> there's that.=C2=A0 The gcc12 port I have can't cope with the =
sample blocks<br>
> code I found on Wikipedia:<br>
> /* blocks-test.c */<br>
> #include <stdio.h><br>
> #include <Block.h><br>
> /* Type of block taking nothing returning an int */<br>
> typedef int (^IntBlock)();<br>
> <br>
> IntBlock MakeCounter(int start, int increment) {<br>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0__block int i =3D start;<br>
> <br>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return Block_copy( ^(void) {<br>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0int ret =
=3D i;<br>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0i +=3D in=
crement;<br>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return re=
t;<br>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0});<br>
> <br>
> }<br>
> <br>
> int main(void) {<br>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0IntBlock mycounter =3D MakeCounter(5,=
2);<br>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0printf("First call: %d\n", =
mycounter());<br>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0printf("Second call: %d\n",=
mycounter());<br>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0printf("Third call: %d\n", =
mycounter());<br>
> <br>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* because it was copied, it must als=
o be released */<br>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0Block_release(mycounter);<br>
> <br>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return 0;<br>
> }<br>
> <br>
> Our current clang is OK:<br>
> % clang -fblocks a.c -o a -lBlocksRuntime<br>
> %<br>
> <br>
> But there's no current users of __block in the kernel. There's=
no<br>
> kernel-specific Block.h file,<br>
> there's no references to BlockRuntime anywhere in the kernel tree =
and<br>
> the code in<br>
> contrib/llvm-project/compiler-rt/lib/BlocksRuntime is completely<br>
> userland specific. There<br>
> is no kernel support that I could see, since we don't have a<br>
> libkern/OSAtomic.h. I'm happy<br>
> to be corrected on this though: I've never tried to use blocks in =
the<br>
> kernel and this is grep<br>
> level confidence.<br>
> <br>
> Clang also doesn't enable blocks unless you pass it -fblock, so yo=
u'd<br>
> need to change a fair<br>
> portion of the kernel build system to enable that.<br>
> <br>
> So I'm thinking regardless of whether or not the project should do=
<br>
> this, you'll have a fair amount<br>
> of choppy waves ahead of you before you could get to the point of<br>
> starting the ifnet work.<br>
> <br>
> Warner<br>
<br>
Hi Warner,<br>
<br>
I did a very very simple test to see what is required link-wise for<br>
blocks in kernel.=C2=A0 This was done by changing<br>
<a href=3D"https://reviews.freebsd.org/D38962" rel=3D"noreferrer" target=3D=
"_blank">https://reviews.freebsd.org/D38962</a>; to use a block instead of a=
callback<br>
for the "bootpc_init_count_if_cb".=C2=A0 I didn't include Blo=
ck.h or<br>
anything, and simply added "-fblocks" to <a href=3D"http://kern.m=
k" rel=3D"noreferrer" target=3D"_blank">kern.mk</a>.=C2=A0 The result is it=
<br>
compiles fine, then fails to link (expected) with the following missing<br>
symbols:<br></blockquote><div><br></div><div>As a basic test, that's fi=
ne. But I'm not sure we want to globally add -fblocks</div><div>to the =
kernel. I don't know if that changes anything else. People will want</d=
iv><div>to know if there's global performance or size impact from doing=
this and</div><div>whether or not the compiler inserts other code because =
blocks are possible.</div><div><br></div><blockquote class=3D"gmail_quote" =
style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);pa=
dding-left:1ex">
_Block_object_dispose<br>
_Block_object_assign<br>
_NSConcreteStackBlock<br>
<br>
Reading through<br>
contrib/llvm-project/compiler-rt/lib/BlocksRuntime/runtime.c these<br>
missing symbols look straightforward to implement for the basic case.<br>
I'm not thinking of working within the Clang runtime.c, I'm thinkin=
g of<br>
reimplementing the needed functions for this constrained use case (no<br>
need for GC, etc).<br>
<br>
This testing was only marginally more than you did, so I'm probably<br>
missing some things as well for more complex use cases.<br></blockquote><di=
v><br></div><div>I was worried about two things when I looked at the code: =
reference counting</div><div>(which I think is kinda required, even though =
objc uses it for GC) and memory</div><div>allocation / handling. The former=
is well understood, and we can adapt things</div><div>(though knowing whic=
h subset is required here might be tricky, there's a lot</div><div>of f=
lags). The latter, though, would limit the use of these APIs to situations<=
/div><div>where you can call malloc/free M_WAIT, or you'd need to deal =
with malloc</div><div>failures better than runtime.c does.</div><div><br></=
div><div>So while the number of routines is small, I think they are the tip=
of the iceburg</div><div>and may be more work than you're suggesting.<=
/div><div>=C2=A0</div><blockquote class=3D"gmail_quote" style=3D"margin:0px=
0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
I'm guessing from GCC's issues that this is a nonstarter anyway?<br=
></blockquote><div><br></div><div>In the past we've said that it's =
OK to use clang specific code to get better</div><div>performance, but that=
depending on it entirely would require a careful</div><div>discussion. gcc=
is produces code that's easier to debug than clang (though</div><div>g=
cc12 build is currently still broken), and that's not nothing and has b=
een</div><div>useful for me in the past. jhb can likely speak to other bene=
fits for gcc12</div><div>since he did the last round of updates.</div><div>=
<br></div><div>So given the difficulties on multiple fronts, I'm not su=
re it's a great idea.</div><div>But maybe I'm wrong about how diffi=
cult things will be and maybe it would</div><div>work out in the end. But s=
electing the network stack to use what will be</div><div>an unproven, or at=
least immature technology is ambitious. We've had a</div><div>mixed ba=
g with that stuff (see epoch and smr for examples).=C2=A0</div><div><br></d=
iv><div>I'm trying hard not to say a flat out "no," because I=
know that sometimes</div><div>things that look hard like this pay off. But=
no gcc support does make it</div><div>really hard to say yes. I've had=
my say, and I'll let others say from</div><div>here.</div><div><br></d=
iv><div>Warner</div></div></div>
--000000000000489b0605f7064b0f--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfqohTcujkDx=rizAXYAKXaDDw09g_hK6WSZ1JGKvvGOgA>