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:   Fri, 2 Dec 2022 17:14:27 -0800
From:   Dan Williams <dan.j.williams@intel.com>
To:     Ira Weiny <ira.weiny@intel.com>,
        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: <638aa2f3349e_3cbe02943d@dwillia2-xfh.jf.intel.com.notmuch>
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>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <Y4qPAi/DZAyz/Djl@iweiny-desk3>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?b4msB/biVAYdLottm85Bq1U5fS6V88ZdKtbdyY50DRBbP7aAMMZVXZIS/kSL?=
 =?us-ascii?Q?2VeUeUsIFaLvo1cfr1X6VByVizjPIfVkdfEPrX0cZ0Gslt5vTFxfWqgo/Fak?=
 =?us-ascii?Q?0Ce1d6hl7qRjuLr1R/QkCz7K4B2hJOAO2VZfWFE6Agd+6QX+vhNiYTLkRhPE?=
 =?us-ascii?Q?IjN/cEESJZ0IpNWInmfbuX5TBn3+MEhsL835yArWEF1A98qFiioawIXYMVt9?=
 =?us-ascii?Q?Kn5PBWtgy7ovDXI3Bc8//QAUnS/GS4UcENYX3MO0I5Lo8oQ/3imne6IPtOyO?=
 =?us-ascii?Q?rQI5APYS6/Wh2sL2XCjeSnAI6WlP9dgytRnYxhKwOnMyX3mZCnFC/q9FxKnf?=
 =?us-ascii?Q?EIBMrsCO3KDBjtag7hvbeFNzxGjITHC4iTD+1Wty0auUBb17RLV2h6YR06V5?=
 =?us-ascii?Q?J0E1ZrsO4gmq0x+E2GhBbWIsHqN2oXBrDfKhAVFWo/NV+0J42o4+3edXiVy0?=
 =?us-ascii?Q?ST++qZZEyobVJM9tdyjhWdXci61ooivlUMfZQmgYYeF5mQB/ojkEFtuLbx4F?=
 =?us-ascii?Q?8y+rl5GENgECOJcwNs4EMWcVQ4h5i35CHRZ1xLavvHEEWZskymG2qCDCWloC?=
 =?us-ascii?Q?0KIZwYDEPBucds3zDnJrXk2epcVRNeurA5GhuaUXy6z8qGUHCfytt65h6ZVA?=
 =?us-ascii?Q?ycagmKmzSOe624/nl/fs4BIApY+y5iH/Be8vDg8LYgtHj6hp7E+/9zX48Mm+?=
 =?us-ascii?Q?PCN5jL4hY29OxZQFpHMpL4jiEheeapI7FMzlhSQXbRjdL+fUyNoj27uGriXc?=
 =?us-ascii?Q?ev+kg1ozpIDUoopMAIu3Aa6JV8dSQf5nqJlP6qzgjjN8PriCvLe6ADggWHTJ?=
 =?us-ascii?Q?myF7T6I2K+8eO4Yk48bJrgBL4CWmA/ubAETwbtQ2dpiCOkAqB6xTDmedJYi5?=
 =?us-ascii?Q?8NqrEv2IgSncInbeNmXVfvBxNZne8sGvDCqX4zwQzPmBLAO3UBExmY8jPA+G?=
 =?us-ascii?Q?Zs+Bfu6F0MdlnlvHCjcP2fksGpkGXsq/6OSVx5Wvrd3cOgVJ0AvPNezz59d8?=
 =?us-ascii?Q?wSkHLPnHpcjM9A6TVTQErA79wM1ZJ8la2rhT/jTj/rRuZwaW30uCvPDs8GWs?=
 =?us-ascii?Q?0pujX9D6STNhwBRY8ISPC9/mU493Cf+yst7oD3fUGGREJQrGjbAcqlFETMfv?=
 =?us-ascii?Q?esYrllclF8m0HTkM7R+hH8Q4nuY60KIrPlrTVkH6XSuTm8bEmWSM8w5Q0I6Y?=
 =?us-ascii?Q?U6LF/O+0Mk0/2q7bJ3nTObhHgFM40UPj1bDVOIIr7UdUMyYWTfJig8zee4EQ?=
 =?us-ascii?Q?vhd+8WnpGzoGigKnXe4S7RW3r1BNKZd0ob6FzOktMiiRQMELCO4gXuNAkAgY?=
 =?us-ascii?Q?rW0vmfJ7RF3eav2RYqPYcavu7nZD0sUsPQMI0AvANvoAUg7VNbjNxoAz7zI3?=
 =?us-ascii?Q?57vC44bSkm8OHoQMf3FaR8iilhN92Q4YPNPFAcuL/XX3XZF2x/kc0yq6E+EA?=
 =?us-ascii?Q?vh5xuMoJJ5eAJmyscszSq856q9YJnYfvq7qbB2yOYCPn0PfIuxiUtloACP2t?=
 =?us-ascii?Q?8SgYEn6TQ5R3+Mvdt5r+GD1LmZW8SbwxM5U3au241pvUi3Kt+vnhvmGPDhGC?=
 =?us-ascii?Q?FdTKWZrBJNG5tVHWkLKStOEPnfg8yORbJ7ujaihxqNwSknbvXi9vVjPiojsi?=
 =?us-ascii?Q?aw=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 31ad252f-c3b7-45bc-36e0-08dad4cbba1a
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1101MB2126.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Dec 2022 01:14:29.3951
 (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: qNjYRzz9tZR5DMuYa40OqZ45yO5npY5LUbYOdzguRIDk+OXgbREcNsG8NtnYNuNVEtCa2LNsARylBmGQjA4q7+qZfMFz8hZ+zf8ilbvqKfs=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR11MB7417
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

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.