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:   Wed, 16 Nov 2022 13:10:49 -0800
From:   Dan Williams <dan.j.williams@intel.com>
To:     Ira Weiny <ira.weiny@intel.com>, Bjorn Helgaas <helgaas@kernel.org>
CC:     Lukas Wunner <lukas@wunner.de>,
        Dan Williams <dan.j.williams@intel.com>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Gregory Price <gregory.price@memverge.com>,
        Jonathan Cameron <Jonathan.Cameron@huawei.com>,
        Alison Schofield <alison.schofield@intel.com>,
        Vishal Verma <vishal.l.verma@intel.com>,
        Ben Widawsky <bwidawsk@kernel.org>,
        <linux-cxl@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <linux-pci@vger.kernel.org>
Subject: Re: [PATCH] PCI/doe: Fix work struct declaration
Message-ID: <637551d941799_12cdff294f6@dwillia2-xfh.jf.intel.com.notmuch>
References: <20221116100939.GA32050@wunner.de>
 <20221116182037.GA1127308@bhelgaas>
 <Y3VO07WbMI5EYAUD@iweiny-mobl>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <Y3VO07WbMI5EYAUD@iweiny-mobl>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?Z+8nQYdLiP91+t7p8v1pyqfuKckJCOcPpQOr2pG0s6Xve0MxHjrC4hlTuNM6?=
 =?us-ascii?Q?pwADVoDwFuP2WULWQdZrjMMRTAcqqzpD4K4m5wGokOr39iSWANg3ky4SYry6?=
 =?us-ascii?Q?S6xRAyuH2Se7d4UJa7GskXMbitUEuu48q4PtVKeXkmm9PEdQVS2rPKhySZ6/?=
 =?us-ascii?Q?cO0tjsYW1kFVkPr4MR9wQeQGuUiICQDCDSVKK68zNmVSLKAYHhXKX2e2KMAh?=
 =?us-ascii?Q?3sXx9gHw0+/BFv5oCy2SGMienPdUNT1BPcZD9MtT/4+kyRDdZGDSX2Ne+QBz?=
 =?us-ascii?Q?ckE7oNvcJgTShYQIvCRY2n40txXQdxbmHIAtsXvG8eBWmRr83Jwed9q607qQ?=
 =?us-ascii?Q?FhHGejk7BD7TNZlo8035nxT9XKuI+z9xnJeaNaq8GkFElV9qCNC+4Glgy73f?=
 =?us-ascii?Q?Kfl3AIMdVpF2sWjge/zOY2c5ln5usTAstLWVVEjtLE7guW5RNX20iyJxOTi2?=
 =?us-ascii?Q?05rsV7y+BjrgriGW20LZ6LB72rhmMAIMDiNDFOQQ1cgzUUpa0I2cXJh6eObb?=
 =?us-ascii?Q?9BZPMpE/GjKZ8IDq/xF0fa39eK3v3QOInfvmHC6OdOZ42wj7A386iWLpueLX?=
 =?us-ascii?Q?6Ip9Kh8sXASPIRhyiyBzPmScNCI72u14w7TAjTxDkomRRfAmPg9ypjiwqgF0?=
 =?us-ascii?Q?RhNpEmV02cMf9JLu4/BVaV0sbtGuB6DUCV8AWH67u22JmRkKmS3ht6LSj17N?=
 =?us-ascii?Q?Wk9XbBDNwXQvzLjZF7j+sXFE6f1UaHf7Oz+hGVN89LAfvlekb0mcyOqPEF8L?=
 =?us-ascii?Q?dDsE8QcCfiQcI7CnFhoKISwFTbGmmzvw/Oq2xF3Kyh1bIQAo83GyuxYj/LH6?=
 =?us-ascii?Q?41Ym4V7t65rVQRaWDbE9HRKgS/ZW/GjRN3rzZaJ5llMIvT/HlRaqOXn/nho5?=
 =?us-ascii?Q?iXT02sk2GE1WciK7pLaDhMY2zaVTroGvahcnUC1D8AJ6LS8mxNr04SDzssX0?=
 =?us-ascii?Q?7/qVR66TtjnGM41gmildtIzTxraRoIZMWVvftJddfN4pEMYVEx0R5U26hYl3?=
 =?us-ascii?Q?s0+ZT0nuIkOV3X5pTn02dTPUndeUF+NFsO5T3jsIs2wXTR+T37IW+YNnP7pv?=
 =?us-ascii?Q?vNkb9h7vwlBCBf+ZJ1wVWsPlUlghzdqVEtifmo7dOgk3FpSpgvE29yvLuTE0?=
 =?us-ascii?Q?m8D7vhX1N6UAqX7N9IhIDfmdWTxrgdbw18r8Te8OjMkzHmGHJKMpQVfcIxjN?=
 =?us-ascii?Q?iA4zUOaXgUHh4xBM4WZVWACLUEnclxVMLTE9Ff19zH6YVY6hNjWYjrYdxOut?=
 =?us-ascii?Q?jOtd8O3bK6bMwtUi4HRgSlfYjTjBptbR334MkoACYO9HL7u06+WYSHDDmCQS?=
 =?us-ascii?Q?+k19PyCBazZCeXc/MOVXeXxnMk0Nco+MJ2LJ2IJHRuSMVReUSeRhRcETi3BJ?=
 =?us-ascii?Q?tIOC2NV7RZQUnJ6MzEw2mco/hhY4BKNoCJOBQzR4ac5JNTBSPcfnEOvQ49yw?=
 =?us-ascii?Q?6EQOhUcjuP7wDZvTpd6kBPrnE6dbnHCssWiK843AuthSDPeC/5rRPT6OIiII?=
 =?us-ascii?Q?qi0z9MJk5pmi1fp8zyJdKOp2DkURxfIKvbVk2dYrk24TKrD0LtGSIE0m8vuq?=
 =?us-ascii?Q?BW3yyfhiFY5aAWASyzjqC6N2OqeiRlIa5ID65UFWz8Mb64qtWnb8BmrhPPrQ?=
 =?us-ascii?Q?Jw=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 55534dc3-dac7-4421-b71e-08dac8170a99
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1101MB2126.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Nov 2022 21:10:51.5401
 (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: 9OXSXPE37zuCdrzQdVV/Ehpx/TShMbKKkM8JFcRNYlf8bYgTFPJXkyjWerkIX+rNPNrY6UccWLaaEls9DL2U1VFb7n79mEltc1XjFSD/t9k=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA0PR11MB4624
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,
        RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,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

Ira Weiny wrote:
> On Wed, Nov 16, 2022 at 12:20:37PM -0600, Bjorn Helgaas wrote:
> > On Wed, Nov 16, 2022 at 11:09:39AM +0100, Lukas Wunner wrote:
> > > On Mon, Nov 14, 2022 at 05:19:43PM -0800, ira.weiny@intel.com wrote:
> > > > From: Ira Weiny <ira.weiny@intel.com>
> > > > 
> > > > The callers of pci_doe_submit_task() allocate the pci_doe_task on the
> > > > stack.  This causes the work structure to be allocated on the stack
> > > > without pci_doe_submit_task() knowing.  Work item initialization needs
> > > > to be done with either INIT_WORK_ONSTACK() or INIT_WORK() depending on
> > > > how the work item is allocated.
> > > > 
> > > > 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.
> > > > 
> > > > Compromise with an intermediate step to initialize the task struct with
> > > > a new call pci_doe_init_task() which must be called prior to submit
> > > > task.
> > > > 
> > > > [1] https://lore.kernel.org/linux-cxl/20221014151045.24781-1-Jonathan.Cameron@huawei.com/T/#m88a7f50dcce52f30c8bf5c3dcc06fa9843b54a2d
> > > > [2] https://lore.kernel.org/linux-cxl/20221014151045.24781-1-Jonathan.Cameron@huawei.com/T/#m63c636c5135f304480370924f4d03c00357be667
> > > 
> > > We have object_is_on_stack(), included from <linux/sched/task_stack.h>.
> > > 
> > > So you could just autosense in pci_doe_submit_task() whether
> > > pci_doe_task is on the stack and call the appropriate INIT_WORK
> > > variant.
> > 
> > Nifty, I had no idea object_is_on_stack() existed, thank you!
> 
> Indeed!  Neither did I!  thanks!
> 
> > 
> > I wonder if there's an opportunity to use object_is_on_stack()
> > somewhere in the INIT_WORK() path to find usage mistakes.
> 
> I'm thinking we could make INIT_WORK do the right thing all the time.  Not sure
> what the overhead of object_is_on_stack() is.
> 
> > 
> > Adding it in pci_doe_submit_task() would add some complexity, so I'm
> > not sure whether it's worth adding it unless we actually have uses for
> > both cases.
> 
> I think if we don't do something we have to document that
> pci_doe_submit_task() only works with tasks on the stack.
> 
> I would rather just make pci_doe_submit_task() correct and not complicate the
> callers.  object_is_on_stack() can't be enough overhead to be worried about in
> this call path can it?
> 
> Actually after writing all that I wonder if we can't push the use of
> object_is_on_stack() into the debug code?  Something like below (completely
> untested)?  I think this could be pushed even further down but I'd like to get
> opinions before attempting a change which will have a wider blast radius.

This looks reasonable, but I would do it after and independently of
introducing the autosensing version of pci_doe_submit_task(). Then you
can pursue this line of thinking and come back to simplify
pci_doe_submit_task() if it indeed moves forward.