Re: [NDCTL PATCH v3] cxl/region: Add -f option for disable-region

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

On 11/27/23 02:34, Cao, Quanquan/曹 全全 wrote:
> 
> 
>> +static int disable_region(struct cxl_region *region)
>> +{
>> +    const char *devname = cxl_region_get_devname(region);
>> +    struct daxctl_region *dax_region;
>> +    struct daxctl_memory *mem;
>> +    struct daxctl_dev *dev;
>> +    int failed = 0, rc;
>> +
>> +    dax_region = cxl_region_get_daxctl_region(region);
>> +    if (!dax_region)
>> +        goto out;
>> +
>> +    daxctl_dev_foreach(dax_region, dev) {
>> +        mem = daxctl_dev_get_memory(dev);
>> +        if (!mem)
>> +            return -ENXIO;
>> +
>> +        /*
>> +         * If memory is still online and user wants to force it, attempt
>> +         * to offline it.
>> +         */
>> +        if (daxctl_memory_is_online(mem)) {
>> +            rc = daxctl_memory_offline(mem);
>> +            if (rc < 0) {
>> +                log_err(&rl, "%s: unable to offline %s: %s\n",
>> +                    devname,
>> +                    daxctl_dev_get_devname(dev),
>> +                    strerror(abs(rc)));
>> +                if (!param.force)
>> +                    return rc;
>> +
>> +                failed++;
>> +            }
>> +        }
>> +    }
>> +
>> +    if (failed) {
>> +        log_err(&rl, "%s: Forcing region disable without successful offline.\n",
>> +            devname);
>> +        log_err(&rl, "%s: Physical address space has now been permanently leaked.\n",
>> +            devname);
>> +        log_err(&rl, "%s: Leaked address cannot be recovered until a reboot.\n",
>> +            devname);
>> +    }
>> +
> 
>>   static int do_region_xable(struct cxl_region *region, enum region_actions action)
>>   {
>>       switch (action) {
>>       case ACTION_ENABLE:
>>           return cxl_region_enable(region);
>>       case ACTION_DISABLE:
>> -        return cxl_region_disable(region);
>> +        return disable_region(region);
>>       case ACTION_DESTROY:
>>           return destroy_region(region);
>>       default:
> 
> Hi Dave
> 
> In this patch, a new function 'disable_region(region)' has been added. When using the 'cxl destroy-region region0 -f' command, there's a check first, followed by the 'destroy-region' operation. In terms of user-friendliness, which function is more user-friendly: 'cxl_region_disable(region)' or 'disable_region(region)'?
> 
> Attach destroy_region section code
> static int destroy_region(struct cxl_region *region)
> {
>     const char *devname = cxl_region_get_devname(region);
>     unsigned int ways, i;
>     int rc;
> 
>     /* First, unbind/disable the region if needed */
>     if (cxl_region_is_enabled(region)) {
>         if (param.force) {
>             rc = cxl_region_disable(region);
>             if (rc) {
>                 log_err(&rl, "%s: error disabling region: %s\n",
>                     devname, strerror(-rc));
>                 return rc;
>             }
>         } else {
>             log_err(&rl, "%s active. Disable it or use --force\n",
>                 devname);
>             return -EBUSY;
>         }
>     }
> 
> I have considered two options for your reference:
> 
> 1.Assuming the user hasn't executed the 'cxl disable-region region0' command and directly runs 'cxl destroy-region region0 -f', using the 'disable_region(region)' function to first take the region offline and then disable it might be more user-friendly.
> 2.If the user executes the 'cxl disable-region region0' command but fails to take it offline successfully, then runs 'cxl destroy-region region0 -f', using the 'cxl_region_disable(region)' function to directly 'disable region' and then 'destroy region' would also be reasonable.

To make the behavior consistent, I think we should use disable_region() with the check for the destroy_region() path.

What do you think Vishal?
> 
> 
> 
> 
>