Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 22 Nov 2022 12:22:48 -0800
From:   Dan Williams <dan.j.williams@intel.com>
To:     Lukas Wunner <lukas@wunner.de>,
        David Laight <David.Laight@aculab.com>
CC:     "'ira.weiny@intel.com'" <ira.weiny@intel.com>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Dan Williams <dan.j.williams@intel.com>,
        Bjorn Helgaas <helgaas@kernel.org>,
        Gregory Price <gregory.price@memverge.com>,
        "Jonathan Cameron" <Jonathan.Cameron@huawei.com>,
        Vishal Verma <vishal.l.verma@intel.com>,
        "linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-cxl@vger.kernel.org" <linux-cxl@vger.kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>
Subject: Re: [PATCH V2] PCI/DOE: Detect on stack work items automatically
Message-ID: <637d2f98db8b_12cdff29464@dwillia2-xfh.jf.intel.com.notmuch>
References: <20221118000524.1477383-1-ira.weiny@intel.com>
 <e59f83f3ca4149d098efe43b48fecd1b@AcuMS.aculab.com>
 <20221122171309.GA11310@wunner.de>
 <20221122171406.GC11310@wunner.de>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20221122171406.GC11310@wunner.de>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?0a74YU8kO3m9MCHDIyzAFpJfRvh/sskGs56nT2nOdRNesnkDT4xnlLNmdpy9?=
 =?us-ascii?Q?2eoCCuRPnpbSIoZrBJE/oFqwYauAs5SuMA7ppGbTNcm0QzHdHllFO7uSJAQa?=
 =?us-ascii?Q?s87gUchbhHrH+PC5npi2Hfb36oPh9aLmRfmDKoQMAZBFo6aTnyEYFgt4lhPt?=
 =?us-ascii?Q?l9vyFKbE+73S8+jMDPv9caO+/aLBZ5gXwSFAfFSz/I+yiSg5kRqNkMgZu+M6?=
 =?us-ascii?Q?808YEertWhRm+XsXSbCbJuue9daw+4cL3DKFxIL8/AVlE0ntbNB/K8alPSGX?=
 =?us-ascii?Q?YDSxXKD/pp15bAMGxV2nud3d9wFXX6aRPvreGcNfdzCCIOwo4K/JoK0TBS/f?=
 =?us-ascii?Q?Ma5RzELHYiJKXUWjpFoVbcDOOCq9NIM9qPp9GqYdYMSzwtBpD/2gFAONFlFc?=
 =?us-ascii?Q?ReXAr5wNerqTVyDj9kuiU4YrDrX+UzbsH+GpzyDboBfb1xzvKexZ06Y7B0ID?=
 =?us-ascii?Q?kjYMih+0raXBnzCHhbW9z9CdbxbD1jknEa0Gaygc4qfev/jOKCXPwwsc4N+J?=
 =?us-ascii?Q?zL8BcxnNJPkmUqefA15Tt0WnU6Pbg2NzAYIEa7HHkB2mazYTYOhP5zFrXhPj?=
 =?us-ascii?Q?zwLEIXBSE8lCqGT/SbmGJGZ/qKqxAxezcYtKbbz88iztNIlCa6T82y0IpjZI?=
 =?us-ascii?Q?qx002oYqtV9VUarNHhfWG80A/ob8StvFsLI49ID5Ubz4B3yZyANyNA2eZ2pc?=
 =?us-ascii?Q?kNyKAh/ChiIthkOvQp7R7llBGg0oJoGZRPilmlRb06/GqdneLyael6jUJKky?=
 =?us-ascii?Q?awptfmW0B4FESAplQUV7v9PcSsUUUES68rTFN6Hn+HqpUVuCDP6/H3ZBCmby?=
 =?us-ascii?Q?2NpyqlXsCsQ4xzs1WTPvsYBZk9Cu0OZvoZMhlsMgaFx5SfXgkH2dl5HLx2/2?=
 =?us-ascii?Q?DpwWxpg7tUefcNeb5fhmeP5RxLnpKOZQV0LgkIiK8h8lCG2m66YTdMIVKVz5?=
 =?us-ascii?Q?ILn4kruAl0pBxngbWYZkWNuH+YuRZLGQ5LCqwNIoS37iYvH2C1yazFzUgZbh?=
 =?us-ascii?Q?7E7dE6/7rhV5wJo+I8PX17Y8HSeA1BAdwe1H/0QByTvfJTfZha3JB9NrduEi?=
 =?us-ascii?Q?WP2V/4Um/xJiWSumHhwLGaFnvy7UFx8ETLbF4g9YOToUfTzizMoq7kAofxrk?=
 =?us-ascii?Q?B1BhB67udbhJ9RJllT74rm+6G01WuJFJ1tTv5S66gc0CLtREmUHWnyoP7JuB?=
 =?us-ascii?Q?elAi1RTK8bvdRSra0OtNgUufj9OtncHr4hM9luD+pz4f2s8XcAEJFxERzU5p?=
 =?us-ascii?Q?jcNvp+qnHv95A5+enIXQ4A8XdtsO9i4CtxngUY7q8qjo0WnTqE10QgbNj7nC?=
 =?us-ascii?Q?hO3VZkmBatxcYlBrsFY2ubNqAC4suT9ws5BZKGg4/wOszurHad5do+UFQLNp?=
 =?us-ascii?Q?Nn96wbhE6FiceG2kQPlGq6f7U7GpBnij7fk9vOX0Nt6X5GdVPK6rCs0Bk6HX?=
 =?us-ascii?Q?jgFH4GQZT3iFr7eqiv/wV1XbIteR+fWIvYQUrxQWZIulWNRoe2fe6yxjIwO7?=
 =?us-ascii?Q?4VJSjvdRkZI+XKbtWYjZUYMwsTPp0AE/gj6IHyzB1woID8KvSb0gcYupsPpB?=
 =?us-ascii?Q?qwCCD+fdQ3pPlbWJG1RR4hPYqay9/SNVZQtNhwZSXdpM5i+8qX2wo0qfe5iI?=
 =?us-ascii?Q?Bg=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 36b3d563-80dd-4a80-256a-08daccc753d9
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1101MB2126.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Nov 2022 20:22:50.5363
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: XWA4JO0DhaHm8RP+LDQSuJ/UkfGAXGl6Luk1VsKMN9uIiRf8uuZpFQzS99cjj6l1+W0vQHfhxHY1HCCax+3kQKNv+gJbBwnFJHiIG6lyAy0=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR11MB6309
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_NONE,SPF_NONE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Lukas Wunner wrote:
> Now with Thomas added to cc for real.
> 
> On Tue, Nov 22, 2022 at 06:13:09PM +0100, Lukas Wunner wrote:
> > [+cc Thomas Gleixner, author of dc186ad741c1]
> > 
> > On Fri, Nov 18, 2022 at 09:20:38AM +0000, David Laight wrote:
> > > > From: ira.weiny@intel.com
> > > > Sent: 18 November 2022 00:05
> > > > 
> > > > Work item initialization needs to be done with either
> > > > INIT_WORK_ONSTACK() or INIT_WORK() depending on how the work item is
> > > > allocated.
> > > > 
> > > > The callers of pci_doe_submit_task() allocate struct pci_doe_task on the
> > > > stack and pci_doe_submit_task() incorrectly used INIT_WORK().
> > > > 
> > > > Jonathan suggested creating doe task allocation macros such as
> > > > DECLARE_CDAT_DOE_TASK_ONSTACK().[1]  The issue with this is the work
> > > > function is not known to the callers and must be initialized correctly.
> > > > 
> > > > A follow up suggestion was to have an internal 'pci_doe_work' item
> > > > allocated by pci_doe_submit_task().[2]  This requires an allocation which
> > > > could restrict the context where tasks are used.
> > > > 
> > > > Another idea was to have an intermediate step to initialize the task
> > > > struct with a new call.[3]  This added a lot of complexity.
> > > > 
> > > > Lukas pointed out that object_is_on_stack() is available to detect this
> > > > automatically.
> > > > 
> > > > Use object_is_on_stack() to determine the correct init work function to
> > > > call.
> > > 
> > > This is all a bit strange.
> > > The 'onstack' flag is needed for the diagnostic check:
> > > 	is_on_stack = object_is_on_stack(addr);
> > > 	if (is_on_stack == onstack)
> > > 		return;
> > > 	pr_warn(...);
> > > 	WARN_ON(1);
> > > 
> > > So setting the flag to the location of the buffer just subverts the check.
> > > It that is sane there ought to be a proper way to do it.
> > 
> > If object_is_on_stack() is sufficient to check whether a struct
> > is on the stack or not, why doesn't __init_work() use it to
> > auto-detect whether to call debug_object_init_on_stack() or
> > debug_object_init()?
> > 
> > Forcing developers to use a specific initializer for something
> > that can be auto-detected is akin to treating them like kids
> > and telling them "You didn't say the magic word."
> > 
> > What's the point?

I had this initial reaction as well, but INIT_WORK_ONSTACK() documents
an important detail of the object's lifetime. Here are 2 examples of
functions that would become trickier to read if the kernel did a
global s/INIT_WORK_ONSTACK()/INIT_WORK()/

    synchronize_rcu_expedited_queue_work()
    insert_wq_barrier()

...where those take arguments that are known to come from the stack and
be used in async context.