From nobody Thu Jul 7 17:59:29 2022 X-Original-To: freebsd-current@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 646A68D1A5C for ; Thu, 7 Jul 2022 17:59:32 +0000 (UTC) (envelope-from kp@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 "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4Lf41J2MtMz3m1Q; Thu, 7 Jul 2022 17:59:32 +0000 (UTC) (envelope-from kp@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1657216772; 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: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=2vdfWiX6TNpZAUZEdupNt0tSsSUOFuDAPl5Yduf7YlM=; b=F2RMkkOvTReir/VLHlmocX5sEW1uUx3SQ0gsCbDXPCrbcx0JaE5DJ8/o3D9VA4xzk7jalQ vDYaOhirkAsMEOecnvxDKQnn0EXC1KvqhzpisSPPMY2r6NagkeW3LkSpis3wT/Bl2kZdUK 8KMj4VycRnJohYfGV63G4DCP/9n5ddGrOSBT1bkHk7kHNk6yPgrUHMdo7FA23JQNgVAel5 oB7hMaeC03tC34SEeULD6fEaDsaVahkVdH2HiHhf0e6dS7kRpWiymMb0M8We1HdCtahsjB 65nH2tpwMMUIylFsQLyJB8ymrwlHLp2IN+47gAPC5xR8mywfXeFnuQ6A6sTTQw== Received: from venus.codepro.be (venus.codepro.be [5.9.86.228]) (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 (2048 bits) client-digest SHA256) (Client CN "mx1.codepro.be", Issuer "R3" (verified OK)) (Authenticated sender: kp) by smtp.freebsd.org (Postfix) with ESMTPSA id 4Lf41H6x73zXfC; Thu, 7 Jul 2022 17:59:31 +0000 (UTC) (envelope-from kp@FreeBSD.org) Received: by venus.codepro.be (Postfix, authenticated sender kp) id 4E5A62E530; Thu, 7 Jul 2022 19:59:30 +0200 (CEST) From: Kristof Provost To: Steve Kargl Cc: Warner Losh , Ryan Stone , FreeBSD Current Subject: Re: buildkernel is broken Date: Thu, 07 Jul 2022 19:59:29 +0200 X-Mailer: MailMate (1.14r5852) Message-ID: <4FE3FF3B-9F2C-41BF-9F96-B8036055A9F6@FreeBSD.org> In-Reply-To: References: List-Id: Discussions about the use of FreeBSD-current List-Archive: https://lists.freebsd.org/archives/freebsd-current List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-freebsd-current@freebsd.org MIME-Version: 1.0 Content-Type: multipart/alternative; boundary="=_MailMate_6C495EB7-3B28-49EA-B95E-C3A9699F2CFD_=" Content-Transfer-Encoding: 8bit ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1657216772; 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: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=2vdfWiX6TNpZAUZEdupNt0tSsSUOFuDAPl5Yduf7YlM=; b=Fmm2PmmjqS8o+9mqtlm/kMjW6aWPoH+OBDFkAX9gJczyMT3dQm2igSwk4Elby0/17CrfWY +TPtbbPKyfIBnnCvG2vGH9dw9lSrC/0ztEaAiqRJhuZYvA2rOIMUCPFV96zc072UfJeBcR ETqTfoefPWJ6rHJIwndsIUyKaCy3EZmK6Y1I+ryACFDhtlxJ5VCAMIxaw08WedFUJqG1ap iIOYoFW47F3L3WNIx4D0o2fxfI/D94oA9DbYYDB2MeZwybsh+yHEwGEcBZyEK7kTCy8hZX +Gqd9iHIYgQUlJLmUOaRP8JqDk2qiwhLHgpdwudN1cDDLGFUxdjdHNis8WPi0w== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1657216772; a=rsa-sha256; cv=none; b=yADRP5AVMmLKtA57TTpeA7eDptJNR1tkjP3Dt5YqXBTEXvhsgJigWRYPTbGG4I9lwCBVUp jevWrcJq/3OiMBEE4dMf4igxKa5fzGOFpHKWcSke4Fmjqc1mgYY4sN4piW6U1+qtPq4Ph4 Q1B6DsdG5qkTJ6N6heUIzMWeSuxzp3YNRLpdaXjKWCj2qQ8KuuYi06CeUjRra0y5EEVUrR 3/9t3Y1NyII7BbdI6MxpgEBpTJt6zk76WYm3KqeEyH0o390gffGQxqSpnn6d5QFIxi8xMf AFbPJaKiOnAX+rfr0uZMnTSRWa/1dgaF1BbGXNB68cCPBZXOsZv75aviA/HKvQ== ARC-Authentication-Results: i=1; mx1.freebsd.org; none X-ThisMailContainsUnwantedMimeParts: N --=_MailMate_6C495EB7-3B28-49EA-B95E-C3A9699F2CFD_= Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit On 7 Jul 2022, at 19:00, Steve Kargl wrote: > On Thu, Jul 07, 2022 at 10:37:40AM -0600, Warner Losh wrote: >> On Thu, Jul 7, 2022 at 10:37 AM Steve Kargl < >> sgk@troutmask.apl.washington.edu> wrote: >> >>> Thanks, but >>> >>> root[216] git cherry-pick -n 37f604b49d4a >>> fatal: bad revision '37f604b49d4a' >>> root[217] pwd >>> /usr/src >> >> >> git fetch maybe? >> > > A cursory google search suggests that 'git fetch' > works on repositories not single files. > > I did look at the diff associated with 37f604b49d4a. > I am surprised that the commit that broke buildkernel > for me was allowed to be committed. It was posted for review in https://reviews.freebsd.org/D35716 I’ll also point out that this commit works just fine in nearly all of our kernel configs, because there are very few (only one powerpc config, as far as I can tell) that do not have VIMAGE. Arguably we should have a non-VIMAGE kernel config around (probably for amd64) so it’s more likely we spot these issues prior to commit. Arbitrary non-default kernel configs are more likely to see issues like this one. I don’t think that can be avoided. > The fix in > 37f604b49d4a seems rather questionable especially given > that there is no comment about why the macro is expanded > to a zero-trip loop. > I’m not sure how I could have been much more clear than this: VNET_FOREACH() is a LIST_FOREACH if VIMAGE is set, but empty if it's not. This means that users of the macro couldn't use 'continue' or 'break' as one would expect of a loop. I welcome suggestions on how to improve my future commit messages. To rephrase it a bit: VNET_FOREACH() used to be very misleading, in that it was only a loop with options VIMAGE, and empty (so any code within would be its own block, and be executed exactly once, for the only vnet that exists without VIMAGE). That’s fine, unless you want to ‘continue’ or ‘break’ the loop. That worked with VIMAGE (so the issue in the dummynet fix was not seen) but not without it. Kristof --=_MailMate_6C495EB7-3B28-49EA-B95E-C3A9699F2CFD_= Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable

On 7 Jul 2022, at 19:00, Steve Kargl wrote:

On Thu, Jul 07, 2022 at 10:37:40AM = -0600, Warner Losh wrote:

On Thu, Jul 7, 2022 at 10:37 AM Steve Kargl <
sgk@troutmask.apl.washington.edu> wrote:

Thanks, but

root[216] git cherry-pick -n 37f604b49d4a
fatal: bad revision '37f604b49d4a'
root[217] pwd
/usr/src

git fetch maybe?

A cursory google search suggests that 'git f= etch'
works on repositories not single files.

I did look at the diff associated with 37f604b49d4a.
I am surprised that the commit that broke buildkernel
for me was allowed to be committed.

It was posted for review in https://reviews.freebsd.org/D35716

I=E2=80=99ll also point out that this commit works just f= ine in nearly all of our kernel configs, because there are very few (only= one powerpc config, as far as I can tell) that do not have VIMAGE.
Arguably we should have a non-VIMAGE kernel config around (probably for a= md64) so it=E2=80=99s more likely we spot these issues prior to commit. Arbitrary non-default kernel configs are more likely to see issues like t= his one. I don=E2=80=99t think that can be avoided.

The fix in
37f604b49d4a seems rather questionable especially given
that there is no comment about why the macro is expanded
to a zero-trip loop.


I=E2=80=99m not sure how I could have been much more clea= r than this:

VNET_FOREACH() is a LIST_FOREACH if VIMAGE is set, but emp=
ty if it's
not. This means that users of the macro couldn't use 'continue' or
'break' as one would expect of a loop.

I welcome suggestions on how to improve my future commit = messages.

To rephrase it a bit: VNET_FOREACH() used to be very misl= eading, in that it was only a loop with options VIMAGE, and empty (so any= code within would be its own block, and be executed exactly once, for th= e only vnet that exists without VIMAGE). That=E2=80=99s fine, unless you = want to =E2=80=98continue=E2=80=99 or =E2=80=98break=E2=80=99 the loop. T= hat worked with VIMAGE (so the issue in the dummynet fix was not seen) bu= t not without it.

Kristof

--=_MailMate_6C495EB7-3B28-49EA-B95E-C3A9699F2CFD_=--