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:   Mon, 5 Dec 2022 23:35:22 -0800
From:   Ira Weiny <ira.weiny@intel.com>
To:     Dan Williams <dan.j.williams@intel.com>
CC:     Alison Schofield <alison.schofield@intel.com>,
        Vishal Verma <vishal.l.verma@intel.com>,
        Ben Widawsky <bwidawsk@kernel.org>,
        "Steven Rostedt" <rostedt@goodmis.org>,
        Jonathan Cameron <Jonathan.Cameron@huawei.com>,
        Davidlohr Bueso <dave@stgolabs.net>,
        "Dave Jiang" <dave.jiang@intel.com>,
        <linux-kernel@vger.kernel.org>, <linux-cxl@vger.kernel.org>
Subject: Re: [PATCH V2 03/11] cxl/mem: Implement Clear Event Records command
Message-ID: <Y47wuiu2RG1ygH/9@iweiny-desk3>
References: <20221201002719.2596558-1-ira.weiny@intel.com>
 <20221201002719.2596558-4-ira.weiny@intel.com>
 <6389630036769_3cbe02947d@dwillia2-xfh.jf.intel.com.notmuch>
 <Y4qPAi/DZAyz/Djl@iweiny-desk3>
 <638aa2f3349e_3cbe02943d@dwillia2-xfh.jf.intel.com.notmuch>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <638aa2f3349e_3cbe02943d@dwillia2-xfh.jf.intel.com.notmuch>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?NfWxsomyyHSxARZsD52iePpp63X2tifClR5EA6fQSz0UVoqyRKA9Lf4rbItp?=
 =?us-ascii?Q?bOMC1Q1uThujJjpJdkQ3dl/oPEq82mC4WuG3ekaUlM9SQbFhONXSMeMWUrAy?=
 =?us-ascii?Q?74l+ldza8D7AE2PCKiZ4BFAk7hD+B/HEQ4dh1JIF11FQYgmNyvITRogXvRxF?=
 =?us-ascii?Q?NRkPxfBs0NkpbSXm0W00l0rCc/9mxIIMNlkYrI9+AERLa2/eJD/yGtyOnSYD?=
 =?us-ascii?Q?FnWX5DuG8feT2LQd0sW69H/GUhst+76XvcBRMdmsR+JujztIxYaQOx4WTZdp?=
 =?us-ascii?Q?1bBbD/DsK4r1EGcXdZv+Y1bA0bv3E9lD/sK6EkZVxoq0vCedb+Zc4NNyrUHy?=
 =?us-ascii?Q?rM5NBl1lCPMj3bgnippRm/eAnk6SpL3BJw1lWu1Y1E1Zaw3kkMage6ekuzR3?=
 =?us-ascii?Q?FMhsAD1HM6xdL6XFvtoec1BWnqUJNr1yT8xIRbKTiocaknJO2ZE0zjZb3oAs?=
 =?us-ascii?Q?O6OW/4EoQqCUKVM6o6oUTB+6eKacgw1QYNLc+axZvyjCOet4GFRwgPSAvgX7?=
 =?us-ascii?Q?cyJykrNKK0U8sWdVbbcX/Ldff8LcZG6mrjubuVYWcpTq2VWEhC9AfMw8jc3y?=
 =?us-ascii?Q?2bmYFjTL2ffmL5qAk+4XpDKLs7HRvCx5/tLtMqHcJOx5l9tJ0gjsREfKNX7b?=
 =?us-ascii?Q?av1lBYcp9BT/VTc0lJC1WKF71WrM4Wis1Sj5BDGZuNMzVrz4BrEQ8CG7+aO1?=
 =?us-ascii?Q?WtsspXlPTmWL6GjX6HmJYq0MPiAJXiOBlloUDkMU24X0mmrIHoCJuOp3jBMG?=
 =?us-ascii?Q?BBwRp4un6z+xJzgGqtNEXX+0nr7G8TOEetIyBtSyLnbsrjVKUi3iUB4tBid5?=
 =?us-ascii?Q?atSQPR0IlByRzVJi455EUbQz2d9R1vysmghzypwKV+2qvcc1fiWdbr4E2qrB?=
 =?us-ascii?Q?BssvKjOCcLmYLqoMfCgWzN8rznAiMlcMPJY6CsCfq6mmmL9dyc3sspImrz0j?=
 =?us-ascii?Q?PhH7HIRuLRR/8kZ9opzIIQGd+1JygowH7OGqj58N+5s0GigB6P6CFgtYJeL+?=
 =?us-ascii?Q?r1JyPHShcaiNAbkGGmmocnt0n8cSQqTTLlfouwJjYjp4dVR+TN0omBIXcIEu?=
 =?us-ascii?Q?NWJPttbWj0mzLTqZ1qktnOHiGrYopMoREQd/4GvLGjjNmvi8mVxwr/g5mTOq?=
 =?us-ascii?Q?BpNADwLxMBgOIrSEwClrUX1jkoPrdZElaWSay0prlnsVU6EGOOfEKYT/oAU2?=
 =?us-ascii?Q?md68ma12FJnpRHIOVS2I78fytP/wENPJJuVWrGDsPuZbHdZKm/Y2JO7eU86s?=
 =?us-ascii?Q?csiTrGCWuOVD6/vc5D4NloQQHup1iEzHSAVEiVQ82dlHNAiDA+AFvg8s082P?=
 =?us-ascii?Q?HIIUCWW2FJLnw2pYOq8lplk8KnDA8nwaldYGvOD51MTxRI7X6VNlzm9GARh+?=
 =?us-ascii?Q?R92gq0REnzvCKKj5cTNTkpK6I16EI3KEBbv4WosMreNkMlF4eyn+Ufnu/lKN?=
 =?us-ascii?Q?MA0dVYvvQRlyJ+HBV5tTKbdg8chBMElOBFAzJcVSOIBph7+hBSoZipe9e7vN?=
 =?us-ascii?Q?736UtSY8YwIbgYow2Bm1JerhJTz7H0/7kclA29gB0fD2sWsnsSaqOksIbcaz?=
 =?us-ascii?Q?fjxdrmmgrZb7qPjYiTwWq22K5u4GRWQ5paeJ5km7?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 0fb40311-19bc-4ae0-c0ee-08dad75c724a
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Dec 2022 07:35:28.2686
 (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: sMcf9iij3XiM9He6hmz0Zmtq/T0lt0jQPIqyd49PzmYWF059zZAsJMXNqTD0xStQfdCISnqB6+BXeZEtXtX2mA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB7076
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-7.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_HI,
        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

On Fri, Dec 02, 2022 at 05:14:27PM -0800, Dan Williams wrote:
> Ira Weiny wrote:
> > On Thu, Dec 01, 2022 at 06:29:20PM -0800, Dan Williams wrote:
> > > ira.weiny@ wrote:
> > > > From: Ira Weiny <ira.weiny@intel.com>
> > > > 
> > > > CXL rev 3.0 section 8.2.9.2.3 defines the Clear Event Records mailbox
> > > > command.  After an event record is read it needs to be cleared from the
> > > > event log.
> > > > 
> > > > Implement cxl_clear_event_record() to clear all record retrieved from
> > > > the device.
> > > > 
> > > > Each record is cleared explicitly.  A clear all bit is specified but
> > > > events could arrive between a get and any final clear all operation.
> > > > This means events would be missed.
> > > > Therefore each event is cleared specifically.
> > > 
> > > Note that the spec has a better reason for why Clear All has limited
> > > usage:
> > > 
> > > "Clear All Events is only allowed when the Event Log has overflowed;
> > > otherwise, the device shall return Invalid Input."
> > > 
> > > Will need to wait and see if we need that to keep pace with a device
> > > with a high event frequency.
> > 
> > Perhaps.  But yea I would wait and see.
> > 
> > [snip]
> > 
> > > > +static int cxl_clear_event_record(struct cxl_dev_state *cxlds,
> > > > +				  enum cxl_event_log_type log,
> > > > +				  struct cxl_get_event_payload *get_pl,
> > > > +				  u16 total)
> > > > +{
> > > > +	struct cxl_mbox_clear_event_payload payload = {
> > > > +		.event_log = log,
> > > > +	};
> > > > +	int cnt;
> > > > +
> > > > +	/*
> > > > +	 * Clear Event Records uses u8 for the handle cnt while Get Event
> > > > +	 * Record can return up to 0xffff records.
> > > > +	 */
> > > > +	for (cnt = 0; cnt < total; /* cnt incremented internally */) {
> > > > +		u8 nr_recs = min_t(u8, (total - cnt),
> > > > +				   CXL_CLEAR_EVENT_MAX_HANDLES);
> > > 
> > > This seems overly complicated. @total is a duplicate of
> > > @get_pl->record_count, and the 2 loops feel like it could be cut
> > > down to one.
> > 
> > Sure, total is redundant to pass to the function.
> > 
> > However, 2 loops is IMO not at all overly complicated.  Note that the 2 loops
> > do not do the same thing.  The inner loop is filling in the payload for the
> > Clear command.  There is really no way around doing this.
> > 
> > Now that I've had time to think about it:
> > 
> > 	Are you suggesting we issue a single mailbox command for every handle?
> > 
> > That would be a single loop.  But a lot more mailbox commands.
> 
> I was thinking something like this pseudo code
> 
> int tosend = le16_to_cpu(get_pl->record_count);
> int added = 0;
> 
>     for (i = 0; i < tosend; i++) {
>     	add_to_clear(added++);
>     	if (added == MAX)
>     		send_mailbox();
> 	added = 0;
>     }
> 
>     if (added)
>     	send_mailbox();
> 
> ...where it batches and sends every 256 and one more send afterwards for
> any stragglers.

Ok I'm not convinced it makes that much difference but I don't have the
fortitude to try and look at the assembly to argue...  ;-)

Done.

Ira