Re: [PATCH 2/3] cxl/mbox: support aborting the current background operation

[Dave Jiang <dave.jiang@intel.com>]

On 1/14/25 12:33 PM, Davidlohr Bueso wrote:
> On Tue, 14 Jan 2025, Dave Jiang wrote:
> 
>> On 10/21/24 8:18 PM, Davidlohr Bueso wrote:
>>> CXL 3.1 introduced the ability to request that the current on-going
>>> background command be aborted. Add support for this, where the current
>>> policy is for the request to occur whenever a new incoming bg command
>>> wants to run. As such everything is left to user discretion and it
>>> becomes impossible to hog the device/mailbox.
>>
>> Are you trying to say that the patch is changing the current behavior to where every time a new bg command comes in, it will abort the previous one?
> 
> Yes.
Perhaps consider:
Add support for the policy where a new command request will supersede and abort the current running background command. With this new behavior, everything is left to the user's discretion. It will no longer be possible to hog the device/mailbox.

DJ

> 
>>
>>>
>>> The context of doing the cancellation request is the same as the new
>>> incoming command, and will always hold the mbox_mutex, guaranteeing
>>> that any successful cancel does not race with a third thread coming
>>> in and stealing the effort.
>>>
>>> - For Sanitize, the thread doing the will cancel the work, and clean
>>
>> doing the? seems to be missing a word here.
> 
> 'doing the request', will update.
> 
> ...
> 
>>> +/*
>>> + * Return true implies that the request was successful and the on-going
>>> + * background operation was in fact aborted. This also guarantees that
>>> + * the respective thread is done.
>>> + */
>>> +static bool cxl_try_to_cancel_background(struct cxl_mailbox *cxl_mbox)
>>> +{
>>> +    int rc;
>>> +    struct cxl_dev_state *cxlds = mbox_to_cxlds(cxl_mbox);
>>> +    struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
>>> +    struct device *dev = cxlds->dev;
>>> +    struct cxl_mbox_cmd cmd = {
>>> +        .opcode = CXL_MBOX_OP_REQUEST_ABORT_BG_OP
>>> +    };
>>> +
>>> +    lockdep_assert_held(&cxl_mbox->mbox_mutex);
>>> +
>>> +    rc = __cxl_pci_mbox_send_cmd(cxl_mbox, &cmd);
>>> +    if (rc) {
>>> +        dev_dbg(dev, "Failed to send abort request : %d\n", rc);
>>> +        return false;
>>> +    }
>>> +
>>> +    if (!cxl_mbox_background_complete(cxlds))
>>> +        return false;
>>> +
>>> +    if (mds->security.sanitize_active) {
>>> +        /*
>>> +         * Cancel the work and cleanup on its behalf - we hold
>>> +         * the mbox_mutex, cannot race with cxl_mbox_sanitize_work().
>>> +         */
>>> +        cancel_delayed_work_sync(&mds->security.poll_dwork);
>>> +        mds->security.poll_tmo_secs = 0;
>>> +        if (mds->security.sanitize_node)
>>> +            sysfs_notify_dirent(mds->security.sanitize_node);
>>> +        mds->security.sanitize_active = false;
>>
>> Should this line happen before the sysfs notification?
> 
> It is benign as we hold the lock, but yes. I will also abstract this in a helper,
> such that both cxl_mbox_sanitize_work() and cxl_try_to_cancel_background() can
> use it.
> 
> Thanks,
> Davidlohr
>