From owner-svn-src-head@freebsd.org Mon Mar 4 23:13:08 2019 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 9648615269AF; Mon, 4 Mar 2019 23:13:08 +0000 (UTC) (envelope-from rmacklem@uoguelph.ca) Received: from CAN01-QB1-obe.outbound.protection.outlook.com (mail-eopbgr660073.outbound.protection.outlook.com [40.107.66.73]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (Client CN "mail.protection.outlook.com", Issuer "GlobalSign Organization Validation CA - SHA256 - G3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 0A7908D888; Mon, 4 Mar 2019 23:13:07 +0000 (UTC) (envelope-from rmacklem@uoguelph.ca) Received: from QB1PR01MB3537.CANPRD01.PROD.OUTLOOK.COM (52.132.89.15) by QB1PR01MB3764.CANPRD01.PROD.OUTLOOK.COM (52.132.86.220) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.1665.19; Mon, 4 Mar 2019 23:13:05 +0000 Received: from QB1PR01MB3537.CANPRD01.PROD.OUTLOOK.COM ([fe80::89d1:c1e9:cb2:3e66]) by QB1PR01MB3537.CANPRD01.PROD.OUTLOOK.COM ([fe80::89d1:c1e9:cb2:3e66%3]) with mapi id 15.20.1665.020; Mon, 4 Mar 2019 23:13:05 +0000 From: Rick Macklem To: Edward Napierala , Cy Schubert CC: Konstantin Belousov , src-committers , "svn-src-all@freebsd.org" , "svn-src-head@freebsd.org" Subject: Re: svn commit: r344758 - in head/sys/fs: nfs nfsserver Thread-Topic: svn commit: r344758 - in head/sys/fs: nfs nfsserver Thread-Index: AQHU0sh37D5whbm3V0KPeIP62mR5+qX8FWOf Date: Mon, 4 Mar 2019 23:13:05 +0000 Message-ID: References: Message from Edward Napierala of "Mon, 04 Mar 2019 15:26:39 +0000." , <201903042025.x24KP9uU021607@slippy.cwsent.com> In-Reply-To: <201903042025.x24KP9uU021607@slippy.cwsent.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: aae4af59-f316-4841-4f74-08d6a0f6f56c x-microsoft-antispam: BCL:0; PCL:0; RULEID:(2390118)(7020095)(4652040)(8989299)(4534185)(4627221)(201703031133081)(201702281549075)(8990200)(5600127)(711020)(4605104)(2017052603328)(7153060)(7193020); SRVR:QB1PR01MB3764; x-ms-traffictypediagnostic: QB1PR01MB3764: x-microsoft-exchange-diagnostics: =?Windows-1252?Q?1; QB1PR01MB3764; 23:f6VhlT+g76AXzThKtRqCe5GMt/77LdJtk+ROR?= =?Windows-1252?Q?1GjAaMLgpizO+CbJYqtWu3FE4DB+wCBx2ve3A/NleNG45+xH0w4bHA6q?= =?Windows-1252?Q?Pl9w+9LIUG+e/n3hD7FbeoyydmLnmM2gQzhdHAeJDfs08a9h1FB6wLoD?= =?Windows-1252?Q?seYLoOi7S35rcnLmoKxX7hzU97p55OR62RchgzwFagVsu1Isp8czU3S/?= =?Windows-1252?Q?TBx850auhfOBRJRxYkbuUWsNeACHTx4s4rBXNKnKypYYlLYGd4YXRKBV?= =?Windows-1252?Q?6TdMdHJBFTGWlJIhFmZhM7zgkYstTxJnBXRPa9CpVxFsmWzkbFbBni21?= =?Windows-1252?Q?GNb+cD+YStmSfK2gZOsv/WQXVYWNXyscIZOUXrDc0F6JehcWNCzarDfw?= =?Windows-1252?Q?nQaKSMh1niRVuQjKOgMQXQbmGE3cWegI14+9k+j89igMp1ClmGd7wKrY?= =?Windows-1252?Q?qM3mHoKd2ahZth997aIoJ8PncKDcL+zul39i633bkMZRshQdYqkYMviU?= =?Windows-1252?Q?EUSzev+nwRgf3pWLr9sJAyeuEzaawMz2v/F1MJErxUWjP/jlMOMh5y5H?= =?Windows-1252?Q?L6sCewC290nOO60N3+rBfjhSROLgrmwo9zpE+9I239AvlKu6mUS+H4IQ?= =?Windows-1252?Q?kIWeMQwfWVeTQDPT8kbAFX2ZoYGeJbJMZqzPkOwc6Kq/tfUkmcdn2mD9?= =?Windows-1252?Q?rHM2f3GQvKE2wnH0WgaiGbgvmeFGZpqfeidm9IcNc+9CNY2PY533PGqL?= =?Windows-1252?Q?oCoKUpvao4wIGd+gGZlYqjGNs0ZYO3zfzRxfxHKmSvXYIWgOCVmecJai?= =?Windows-1252?Q?B8/lUhrgfQYwx13X/6i/BFDTbe0g02sRy/W3Zal4KKOZiv3Y/fWmLpUr?= =?Windows-1252?Q?wYVliHeefY8LFmPUvMHsDqwbykokdV0p8vnGM7LSwrmDodWfAx20CNJh?= =?Windows-1252?Q?eZQzo/OS8XS/uDJw9ewfV+WqWPBLbtIvjpbkXe0/sPYRag2LSBlSrfDP?= =?Windows-1252?Q?MxWN5FQBBCggDPv76OmfPYSTt+//1TAZZpY61zBr1Uiwzw2i9VytaGIf?= =?Windows-1252?Q?SA26oR4cr7XAdcNxB60zpMrM2fMHcuC3VdX8/HGBujPR1kp/fgYYzwC/?= =?Windows-1252?Q?V8bpOTkPP4DlD2otrYOHFO+C2yTxSG3Stl4kLdDGGZyQ3mDhwrEzLULb?= =?Windows-1252?Q?j4i20J1DHBzVCWCEj6mYXu0BFzegXaK3BkMApQ7LUge34scwTsWw8b27?= =?Windows-1252?Q?ccOuKIvW8bVZ7rjsA=3D=3D?= x-microsoft-antispam-prvs: x-forefront-prvs: 09669DB681 x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(39860400002)(346002)(376002)(366004)(396003)(136003)(199004)(189003)(2906002)(26005)(33656002)(68736007)(25786009)(186003)(305945005)(105586002)(71200400001)(71190400001)(106356001)(4326008)(97736004)(256004)(14444005)(74482002)(6506007)(14454004)(478600001)(6246003)(54906003)(110136005)(86362001)(786003)(316002)(74316002)(53936002)(9686003)(99286004)(6436002)(7696005)(476003)(8676002)(81166006)(81156014)(76176011)(11346002)(55016002)(446003)(8936002)(102836004)(486006)(229853002)(5660300002); DIR:OUT; SFP:1101; SCL:1; SRVR:QB1PR01MB3764; H:QB1PR01MB3537.CANPRD01.PROD.OUTLOOK.COM; FPR:; SPF:None; LANG:en; PTR:InfoNoRecords; A:1; MX:1; received-spf: None (protection.outlook.com: uoguelph.ca does not designate permitted sender hosts) x-ms-exchange-senderadcheck: 1 x-microsoft-antispam-message-info: uldHPSN8/S4/OWptEY08+hT7qGCoZwCoPyV1v8JgFpBBF/dsoVcLILsDJ0LTBHTH5qlcw8zuZbR+/EBKW8Hjk0PjtzE4yJwa63y/v+cMRrqME5tyEwJsCIW+YCOnM9AwZwD5EUDY9LFKb92PFnBBMYqdDp2Gn4BqCJhX8vjF7y/lHkzIL8m5iBE09aTOJ3Akk5Ey7VpRfcjb24FYFD6cemV8RFKHGfMRYHOkSXEIDGLjWKlVsNm/7otk/Ik+mPvFCb2TCFGkcWNniNMBnwX3QAwXiW5mfkUEIP89KYsu//5QedL5a45bkPYT/0XjyqI/qn0ac3X7ZloywSlgNgp9fZsZj9xh/U34ag+tbbftKNXanlOr0NKjKsYRFR6lsrFXMpOaRdzZNMbz4NBKt4VF0Qq4JcQ8lDMJZszX3Qt0J7Y= Content-Type: text/plain; charset="Windows-1252" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: uoguelph.ca X-MS-Exchange-CrossTenant-Network-Message-Id: aae4af59-f316-4841-4f74-08d6a0f6f56c X-MS-Exchange-CrossTenant-originalarrivaltime: 04 Mar 2019 23:13:05.7997 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: be62a12b-2cad-49a1-a5fa-85f4f3156a7d X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-Transport-CrossTenantHeadersStamped: QB1PR01MB3764 X-Rspamd-Queue-Id: 0A7908D888 X-Spamd-Bar: ------ Authentication-Results: mx1.freebsd.org X-Spamd-Result: default: False [-6.90 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; NEURAL_HAM_SHORT(-0.90)[-0.896,0]; REPLY(-4.00)[]; NEURAL_HAM_LONG(-1.00)[-1.000,0] X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 04 Mar 2019 23:13:08 -0000 Cy Schubert wrote: >Sent: Monday, March 4, 2019 3:25 PM >To: Edward Napierala >Cc: Cy Schubert; Konstantin Belousov; src-committers; svn-src-all@freebsd.= org; svn-src->head@freebsd.org >Subject: Re: svn commit: r344758 - in head/sys/fs: nfs nfsserver > >In message , Edward Napierala writes: >> pon., 4 mar 2019 o 15:17 Cy Schubert napisa= =C5=82(a): >> > >> > On March 4, 2019 6:30:21 AM PST, Konstantin Belousov >> wrote: >> > >On Mon, Mar 04, 2019 at 01:31:37PM +0000, Edward Napierala wrote: >> > >> pon., 4 mar 2019 o 13:20 Konstantin Belousov >> > >napisa=C5=82(a): >> > >> > > + p =3D curthread; >> > >> > Why do you name it 'p', which is typical for process, and not 'td= ', >> > >you are >> > >> > changing most of the code anyway. >> > >> >> > >> To keep the diff size smaller. You're right, this touches a lot of >> > >stuff, >> > >> but most of those added lines are temporary anyway - they will be >> > >> removed later, when the td is pushed down even more. >> > >But if you create code churn, doing it only half way is worse. >> > > >> > >> >> > >> > Also I am curious why. It is certainly fine to remove td when it = is >> > >used >> > >> > as a formal placeholder argument only. But when the first action = in >> > >the >> > >> > function is evaluation of curthread() it becomes less obvious. >> > >> >> > >> Again, many/most of those are temporary. I'm trying to push td dow= n >> > >> in small steps, "layer by layer", so it's easy to review. >> > >> >> > >> > curthread() become very cheap on modern amd64, I am not so sure >> > >about >> > >> > older machines or non-x86 cases. >> > >> >> > >> The main reason is readability. Right now there's no easy way to >> > >tell whether >> > >> a function can be passed any td, or if it must be curthread. >> > >I must admit that this is the weirdnest argument against 'td' that I >> > >ever >> > >heard. I saw more or less reasonable argumentation >> > >- that using less arguments make one more register for argument passi= ng >> > > (amd64 has 6 input arg regs), >> > >- that less arguments make smaller call code. >> > >But trust me, in all cases where function can take td !=3D curthread,= it >> > >is >> > >either obvious or well-known for anybody who works with that code. >> > > >> > >Before you start doing a lot of small changes (AKA continous churn) >> > >please formulate your goals and get some public feedback. My immedia= te >> > >question that I want answered before you ever start touching the code= , >> > >is what you plan to do with >> > > sys_syscall(struct thread *td, uap) >> > >> > Agreed on all points. At the very least this group of commits should b= e rev >> iewed on phabricator. >> >> It has been, even though they are pretty much mechanical changes. >> >> > Can we back all these commits out until there is a proper review, plea= se? >> >> The review from the NFS maintainer is not enough? >> Btw, I've never listed myself as the NFS maintainer. I need to go look to s= ee if someone else put me in the maintainer's list. I understand that it is mostl= y authored by me, but I consider it FreeBSD project code once committed. (Others do co= mmits to it without my review and that is just fine with me.) >As it's NFS-only maybe though for anything substantial, like this, the >more eyes the better. > >I'd agree with kib@ that we want to keep the amount of churn down, >though it's understood that you want to separate the functional changes >from the cosmetic ones. I tend to do the review and use git svn to >separate the functional from the cosmetic changes, batching changes if >I can. It's more work but IMO well worth it. This is way too technical for me. I can barely look at a "diff" and make se= nse of it.;-) It sounds like there needs to be a discussion (on freebsd-fs@ perhaps?) of = the "big picture change" here. All I am going to do with the patches in phabricator is take a quick look t= o see if I can spot anything that will break the code. (I did mention that I didn't understand why he was doing this in one of the= reviews, but noted that it didn't break anything.) Oh, and the variable was called "p" because the code started in OpenBSD, wh= ere it was a proc ptr and I kept it portable by replacing "struct proc" with NFSPR= OC_T. (This portability is no longer a consideration.) I'll hold off on further phabricator reviews until the "big picture" change= gets discussed on some mailing list. (I don't see phabricator as the correct tool for "big= picture" discussions.) rick