From nobody Fri Jun 28 13:22:30 2024 X-Original-To: dev-commits-src-main@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4W9bhj6TDLz5Mm9F; Fri, 28 Jun 2024 13:22:45 +0000 (UTC) (envelope-from zlei@FreeBSD.org) Received: from smtp.freebsd.org (smtp.freebsd.org [96.47.72.83]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "smtp.freebsd.org", Issuer "R10" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4W9bhj5hbPz47Ky; Fri, 28 Jun 2024 13:22:45 +0000 (UTC) (envelope-from zlei@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1719580965; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=liFRWJNWT+USNtGSzKV4t9Czw8jhAknM7AutGj8LszY=; b=Z3nCSr15p54qaCjtFML6l/5Z6A/MHGCI346kLNu5QVHIGwDQ5I+UH7IhAr4YrjeSuEYT9h 8ixKSoVYcBkGR4BoGCRL2ZXDSlCeqMvmcrFOq+5BSqOI1OLetzvT//mLp7ph6wW3loaGqy CP8U30PHES9zqdCsg+RlcPG52LylZhoazj/RBCeiS7rV33d3JGuJCJtqbfjNAQpFEq9/61 Hg869u1jPmRs3+4X/sM9hRWtIhGA2oisk+p17xrK5FmvvyHODNzVrjGQHSNmzS76JofLlY egC20aKuM5VDMdXW9HQYXFeRm+/sh0x5FfG3xbn4y/Sp+atJ/+vnltWPY5JUtg== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1719580965; a=rsa-sha256; cv=none; b=MqsNuHXtNZO7e4L3ItAx8rKQCgtKWm8lT9nioliHXiXHclguhRKfDtOfWzwMXq8MtjlhN7 u57NbKrtUlg5xSwPTdFusgqbNfZ5bUKJxMjBIU2rlr9F8xJwjqat2AfVdUT+PPLmlTzUCx 5K//D/9dgKbDWV/iun3OFoAUgewxkrijhpIaceFbjvyGho8+RllNLSgwh4Xi1o4o6n0Dbp VzcOiNC3+OKpUlpq5n63yob+lCmn2R527Wd2ourag1couhBIWnYATot7/uZcNJYn/g6oAJ J6RlWwUfzbmP/KuZy9TycM/KierWpHgtkIG4qQsmOjdBg8JKbHcBO6cpMvnfbw== ARC-Authentication-Results: i=1; mx1.freebsd.org; none ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1719580965; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=liFRWJNWT+USNtGSzKV4t9Czw8jhAknM7AutGj8LszY=; b=WS4kxcXkJih8TPesqfYe1hd8uTJ36hi4gCpc9XCMDG8ywbEbFNSM9F2TP+dgnEgGQTSaT6 46ZQcaz7zVlcY5ttwdlbLNu+3+jQwOwpArYfcgEzBiapQTYCpaEJACFeveAZrTELnAZ/SH ZTafwVWBx6wkjFq07yZQEKEF74alXvpEyDWNOhUmzPuv0OiRyJk3lkaFzvXI7lOw8xWD+2 LGWFqCEWhb5qsYS7H3OVUZpFUqpo9EBYFJFzC8y6x7SMrYwyvy+AC+PfrGuOPHnpmK6ifE soA5OXbgn4H7jLJbrwQeqcYm5LuXO20I36dSclsTnJvTa7y2pChNJ88gdQZ2kQ== Received: from smtpclient.apple (unknown [IPv6:2001:19f0:6001:9db:98f0:9fe0:3545:10]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) (Authenticated sender: zlei/mail) by smtp.freebsd.org (Postfix) with ESMTPSA id 4W9bhg55MLz16g2; Fri, 28 Jun 2024 13:22:43 +0000 (UTC) (envelope-from zlei@FreeBSD.org) From: Zhenlei Huang Message-Id: Content-Type: multipart/alternative; boundary="Apple-Mail=_64E2865D-B60F-40FF-9720-2EB6536D1822" List-Id: Commit messages for the main branch of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-main List-Help: List-Post: List-Subscribe: List-Unsubscribe: X-BeenThere: dev-commits-src-main@freebsd.org Sender: owner-dev-commits-src-main@FreeBSD.org Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3696.120.41.1.8\)) Subject: Re: git: aa3860851b9f - main - net: Remove unneeded NULL check for the allocated ifnet Date: Fri, 28 Jun 2024 21:22:30 +0800 In-Reply-To: <202406281038.45SAcjNd010520@critter.freebsd.dk> Cc: "src-committers@freebsd.org" , "dev-commits-src-all@freebsd.org" , "dev-commits-src-main@freebsd.org" To: Poul-Henning Kamp References: <202406281018.45SAILbb075980@gitrepo.freebsd.org> <202406281038.45SAcjNd010520@critter.freebsd.dk> X-Mailer: Apple Mail (2.3696.120.41.1.8) --Apple-Mail=_64E2865D-B60F-40FF-9720-2EB6536D1822 Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset=us-ascii > On Jun 28, 2024, at 6:38 PM, Poul-Henning Kamp wrote: > > -------- > Zhenlei Huang writes: > >> commit aa3860851b9f6a6002d135b1cac7736e0995eedc >> >> net: Remove unneeded NULL check for the allocated ifnet >> >> Change 4787572d0580 made if_alloc_domain() never fail, then also do the >> wrappers if_alloc(), if_alloc_dev(), and if_gethandle(). >> >> [...] >> diff --git a/sys/arm/allwinner/if_emac.c b/sys/arm/allwinner/if_emac.c >> index f581d361d3d9..1db43cbca26c 100644 >> --- a/sys/arm/allwinner/if_emac.c >> +++ b/sys/arm/allwinner/if_emac.c >> @@ -940,11 +940,6 @@ emac_attach(device_t dev) >> emac_reset(sc); >> >> ifp = sc->emac_ifp = if_alloc(IFT_ETHER); >> - if (ifp == NULL) { >> - device_printf(dev, "unable to allocate ifp\n"); >> - error = ENOSPC; >> - goto fail; >> - } >> if_setsoftc(ifp, sc); >> >> /* Setup MII */ > > I would like to suggest that we use a KASSERT() to document the > expectations in cases like this. > > It will costs us nothing, and it helps tell both human readers of > the source code, the compiler, and code analysis tools what to > expect. I would recommend further, that, to have some compiler hints for those kind of tasks. I think gcc extensions _Nullable _Nonnull could fulfil but I have not tried yet. Other language for example Java also have such means so that the human labor can be freed. There're also other kind of issues, such as do NULL check after malloc with M_WAITOK flag. That is quite common but is more domain specific. I guess simple compiler hints will not help, at least static code analyzer is required. More suggestions are welcomed! > > Background: > > One of the things I did with Varnish Cache was make asserts mandatory > and numerous, to the point where about one source line in ten is an > assert in our code base. > > In Varnish asserts are not compiled out in production code, and so > far nobody has ever documented any performance difference by doing so. > > This is because almost all the asserts are on data already present > in a CPU register at the time, and in many cases the compiler is > even able to determine that the assert is always false and eliminates > it before code generation. > > But having the assert there still clearly states the expectation > for both humans and programs. > > For basic "is(n't) NULL" we have two short-hand assert macros: > > AZ(expression) // is zero > AN(expression) // is not zero/NULL > > Maybe we should have something similar in sys/kassert.h ? > > -- > Poul-Henning Kamp | UNIX since Zilog Zeus 3.20 > phk@FreeBSD.ORG | TCP/IP since RFC 956 > FreeBSD committer | BSD since 4.3-tahoe > Never attribute to malice what can adequately be explained by incompetence. --Apple-Mail=_64E2865D-B60F-40FF-9720-2EB6536D1822 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=us-ascii

On Jun 28, 2024, at 6:38 PM, Poul-Henning Kamp <phk@phk.freebsd.dk> = wrote:

--------
Zhenlei Huang writes:

commit = aa3860851b9f6a6002d135b1cac7736e0995eedc

=    net: Remove unneeded NULL check for the allocated = ifnet

   Change = 4787572d0580 made if_alloc_domain() never fail, then also do the
   wrappers if_alloc(), if_alloc_dev(), and = if_gethandle().

[...]
diff = --git a/sys/arm/allwinner/if_emac.c b/sys/arm/allwinner/if_emac.c
index f581d361d3d9..1db43cbca26c 100644
--- = a/sys/arm/allwinner/if_emac.c
+++ = b/sys/arm/allwinner/if_emac.c
@@ -940,11 +940,6 @@ = emac_attach(device_t dev)
emac_reset(sc);

= ifp =3D sc->emac_ifp =3D if_alloc(IFT_ETHER);
- = if (ifp =3D=3D NULL) {
- = device_printf(dev, "unable to allocate ifp\n");
- = = error =3D ENOSPC;
- goto fail;
- }
= if_setsoftc(ifp, sc);

/* Setup = MII */

I would like to suggest = that we use a KASSERT() to document the
expectations in = cases like this.

It will costs us nothing, = and it helps tell both human readers of
the source code, = the compiler, and code analysis tools what to
expect.

I = would recommend further, that, to have some compiler hints = for
those kind of tasks. I think gcc extensions _Nullable = _Nonnull could
fulfil but I have not tried yet.

Other language for example Java also have such = means so that
the human labor can be freed.

There're also other kind of issues, such as do = NULL check after malloc
with M_WAITOK flag. That is quite = common but is more domain specific.
I guess simple compiler hints will not help, at least = static code analyzer
is = required.

More suggestions are welcomed!


Background:

One = of the things I did with Varnish Cache was make asserts mandatory
and numerous, to the point where about one source line in ten = is an
assert in our code base.

In Varnish asserts are not compiled out in production code, = and so
far nobody has ever documented any performance = difference by doing so.

This is because = almost all the asserts are on data already present
in a = CPU register at the time, and in many cases the compiler is
even able to determine that the assert is always false and = eliminates
it before code generation.

But having the assert there still clearly states the = expectation
for both humans and programs.

For basic "is(n't) NULL" we have two short-hand assert = macros:

AZ(expression) // is = zero
AN(expression) // is not = zero/NULL

Maybe we should have something = similar in sys/kassert.h ?

--
Poul-Henning Kamp       | UNIX = since Zilog Zeus 3.20
phk@FreeBSD.ORG =         | TCP/IP since RFC = 956
FreeBSD committer =       | BSD since 4.3-tahoe =    
Never attribute to malice what can = adequately be explained by incompetence.



= --Apple-Mail=_64E2865D-B60F-40FF-9720-2EB6536D1822--