Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <7c620f8a-189e-5ac4-30fe-1fa14ba799ea@intel.com>
Date:   Mon, 7 Mar 2022 11:55:26 -0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.6.1
Subject: Re: [RFC 08/10] platform/x86/intel/ifs: Add IFS sysfs interface
Content-Language: en-US
To:     Dan Williams <dan.j.williams@intel.com>
CC:     "hdegoede@redhat.com" <hdegoede@redhat.com>,
        "markgross@kernel.org" <markgross@kernel.org>,
        "corbet@lwn.net" <corbet@lwn.net>,
        "Raj, Ashok" <ashok.raj@intel.com>,
        "Luck, Tony" <tony.luck@intel.com>,
        "dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>,
        "patches@lists.linux.dev" <patches@lists.linux.dev>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "mingo@redhat.com" <mingo@redhat.com>,
        "rostedt@goodmis.org" <rostedt@goodmis.org>,
        "Shankar, Ravi V" <ravi.v.shankar@intel.com>,
        "tglx@linutronix.de" <tglx@linutronix.de>,
        "platform-driver-x86@vger.kernel.org" 
        <platform-driver-x86@vger.kernel.org>,
        "linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
        "hpa@zytor.com" <hpa@zytor.com>, "bp@alien8.de" <bp@alien8.de>,
        "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        "andriy.shevchenko@linux.intel.com" 
        <andriy.shevchenko@linux.intel.com>,
        "x86@kernel.org" <x86@kernel.org>
References: <20220301195457.21152-1-jithu.joseph@intel.com>
 <20220301195457.21152-9-jithu.joseph@intel.com>
 <188492dc80c017375da76d444347b1d00c2031f6.camel@intel.com>
 <7b9c788e-21dc-eedc-a1b4-9c6877fa48fe@intel.com>
 <CAPcyv4h=qPFrP+mRqaZhkh5ZmYjuQawsqvf+-R036ZJVKBNK4Q@mail.gmail.com>
 <33d0e764-86d9-8504-17fa-14b31c87de4e@intel.com>
 <CAPcyv4g5bq9+u0iLjhpeJw8bkbCREUw60H2z_KfDz4hHCrKdFQ@mail.gmail.com>
From:   "Joseph, Jithu" <jithu.joseph@intel.com>
In-Reply-To: <CAPcyv4g5bq9+u0iLjhpeJw8bkbCREUw60H2z_KfDz4hHCrKdFQ@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?RDAyTSt6NCt3dlZlQmE4K252em8rM1dTN0pubzlOMGo4OW5LUDQ5d1JSN29K?=
 =?utf-8?B?azl1VHBuUnpKV2s3YnVieXVyZVRHTTFvSUozMXRqN3R4elpRY1ZmNE5EM0JR?=
 =?utf-8?B?eUhqSmhDeGovOXRUZDBZeHZxanAza2lGbzkrSnlEeWtYOVpJMkMyV3J2c0pq?=
 =?utf-8?B?TFkrSDhiNkVFUklPYmo5VVRiZ1ErWGVVdGhzcHVLRVFucTltbjBDMGM1dG9j?=
 =?utf-8?B?enFEUjZPTitHNlFlSitOcXFCSEl0OFZVT28wZit3K2xjL24zTjd2S3g2cGNE?=
 =?utf-8?B?K3dUTisxOFU5YjZlUnFCMWJCTmdYQWFuK3hmUnpmVklNU1QrVTVVbXNLb0tU?=
 =?utf-8?B?RGUzYnRaZTRKcFl2U3JGeitlbXg0Y3BzVlBQcElGZk03bUxoMWFtTmJWVCty?=
 =?utf-8?B?TTNaZTg0R1JPRnB3Z1pvLzR2SkFQa1VxaFBoeXFnWTF4ZUVlTU1pSCtYN3lZ?=
 =?utf-8?B?ZloyOE8rQVFXS29pODc1QWZBOFQrYWdOZFM3TXRKNnFJV2xQdmEwejBDSXFh?=
 =?utf-8?B?YTJNL0VmMUZmTWdxVEd3RlhQcjJ3eE9YWVYrQldBVmFMU0prYmdoaGM0eHhw?=
 =?utf-8?B?QzNjaVNMMGpZcS9QTzBhczkzRmVwMkZwK2JaKzRFVFRQbTNHT3FtQVdXV0tY?=
 =?utf-8?B?MldOdGgzbWV2YzdNSE9kYlg4bHhQSytSWllEYWQzYmN2UDVRMkRUSWhYNEo0?=
 =?utf-8?B?NEhuQ2twc1JWTVZzdFRmWUhiekxRcE95SU5QaWoyQVNUM0lpQUpzWlVZYmwr?=
 =?utf-8?B?N3hRNUZyQk5mQ0svOWxRKzJxRUxYYnhZRXNDUFhzOGpKanluZlR5OHNhc2tG?=
 =?utf-8?B?NkNDWitITHRCcHNoelZmR0dpaDdtRUFsNEZ1VHM2YzMrSXZzaXNkUE5hbEhu?=
 =?utf-8?B?dGZBaUVldnh6OHl3R2trdDB0U2taVEJFTUVQemJWcGpTcU5McVRJMDdUblZV?=
 =?utf-8?B?b3JNQUxkVDN4Y040V2RrVk5DdkFsU1ZLVm1WUmpLMGVPeWpqaE9hNktqcEJx?=
 =?utf-8?B?cC9YV2cwNStPOWVBcTlBOFZaRzgxc3FPOGllNzB2L3hvR0FPZnBaKzJwSysz?=
 =?utf-8?B?NEFqaXUzWHlxa0xuTGdqN1V1dHJBdEJwdjlGclNWbHdmaVhzZzdCY3g4Q2RF?=
 =?utf-8?B?ZTBBQk9UbnBYdDBFM0Jqc3Nsb3JSbDdTTTJqenpibTZOb1NjWE16M1JXbjY2?=
 =?utf-8?B?SGphb2h5eHhhR2R4ZEpieEdUa0JRcm5LdWRTOXY4ZTBZam9LVjVqakhDT1Fn?=
 =?utf-8?B?NEZRVkVDVUR4RkE2Nkt1VDhCdkhReWNnK2huSnJWNmh3T3FkRFVDNkdBTEUv?=
 =?utf-8?B?WUZZeWR0cElBNFp5YVZsVWtLbG1LcXpHZ2Q5V2lhOEgrM2RVaHh6T09YaWpn?=
 =?utf-8?B?Yk5YWXBaeGpaekRMT1AvdDIwanFuYmxNbWVQVEk2Qm04Tkkrc3A0eUdaWFJu?=
 =?utf-8?B?TVpCeVMxUHZHa1h1NlhLTngzTURvL25IVnRsbWdseVRRSEx4Z2g0OFFhOWJ2?=
 =?utf-8?B?WXVwdHcvc0xlSXZJVGJuWjQ4WTA3YkdOLzBhOFRxMmZZZVZFQnJtVWZlNFNC?=
 =?utf-8?B?YkcxTlFGYUF3Vm8xbHJ1eU5haXh3anZrZ0R5MGNuZWxMWk5rQ3RTUU9WWGFS?=
 =?utf-8?B?dE9KNHdYamhrV0lOTDhxRG5TZ1hjUWliZUFxc3ZpZG43V1RVV1ZhMnp5RGZW?=
 =?utf-8?B?Y0UwbzFEUEF0ajR6VjFtUFNKZ3BnS0FJdE4ydDZ2Y1A4NW5maWpoRm13QlRE?=
 =?utf-8?B?N21reDNmMzJNTnovVlYzbmpaV0l1M1lOam1Ib09iWDZlUmtCWUc4b0tha25N?=
 =?utf-8?B?cmNhdXJPTDM3dCtxWkx2ck8yc2lHM3hvTTNHT0xJOTFlUW84aVF4RkNYUnZN?=
 =?utf-8?B?WVNITXVEL2crdENrZnZpdlRmRVJWTGQzY09tR2Q3ZWcxUC9iYkE0R2w2b3FK?=
 =?utf-8?B?TFFpL0lJT0UycEJhTjdEODBBMUxBM2pDT0FxNm9kZkI4cWVwK2dwK2U3ZnRE?=
 =?utf-8?B?b3RaanpuTE92bndkbWRPeksvZncya1Nham1aR2ZQNkw4d0NrQjN4VWdYNFNM?=
 =?utf-8?B?bFVJcXNpeFFNSTNucnFGcytBSDR6cG9JclYxZ1ppdnhrdm5iZ2FKaklCd0Jh?=
 =?utf-8?B?ei9VOHJTUXhBYkErejh1cDc3TDNFZWwreEs4U3dYdS9yVWpWQkUvbFdodVBu?=
 =?utf-8?Q?CZ9pmoko1e0LpEYGEaEQtJQ=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 434ae4e7-ed50-4f27-79cc-08da00746f86
X-MS-Exchange-CrossTenant-AuthSource: BYAPR11MB3768.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Mar 2022 19:55:31.6605
 (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: uL3XtrY6bzEcv1n1woXdYTSa4cXl5pfUVaHhIeoM5LPUzOShrQPrRXvXMk4EXFdd67mMu8LaI27rCW/gIRs3LA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR11MB2680
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
	DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,RDNS_NONE,SPF_HELO_NONE,
	T_SCC_BODY_TEXT_LINE autolearn=unavailable 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 3/7/2022 11:15 AM, Dan Williams wrote:
> On Mon, Mar 7, 2022 at 11:09 AM Joseph, Jithu <jithu.joseph@intel.com> wrote:
>>
>>
>>
>> On 3/7/2022 9:38 AM, Dan Williams wrote:
>>> On Fri, Mar 4, 2022 at 12:42 PM Joseph, Jithu <jithu.joseph@intel.com> wrote:
>>>>
>>>>
>>>>
>>
>>>>>> + */
>>>>>> +static ssize_t details_show(struct device *dev,
>>>>>> +                           struct device_attribute *attr,
>>>>>> +                           char *buf)
>>>>>> +{
>>>>>> +       unsigned int cpu = dev->id;
>>>>>> +       int ret;
>>>>>> +
>>>>>> +       if (down_trylock(&ifs_sem))
>>>>>> +               return -EBUSY;
>>>>>
>>>>> What is the ifs_sem protecting? This result is immediately invalid
>>>>> after the lock is dropped anyway, so why hold it over reading the
>>>>> value? You can't prevent 2 threads racing each other here.
>>>>
>>>> percpu thread running scan_test_worker() will update per_cpu(ifs_state, cpu).scan_details. (before signalling this thread to run, this lock would be acquired)
>>>> This is to protect against the scenario where if the percpu thread is running a test and if at the same time a user is querying its status, they would see busy.
>>>
>>> That begs the question why would userspace be polling this file? Is it
>>> because it does not know when a test completes otherwise? How does it
>>> know that the result it is seeing is from the test it ran and not some
>>> other invocation to start a new test?
>>
>> I think I should have explained the need for locking at a higher level .
>> It is to make sure that only one of the below action happens at any time
>>
>> 1. single percpu test
>> 2. all-cpu test (tests all cores sequentially)
>> 3. scan binary reload
>> 4. querying the status
>>
>> (There are h/w reasons for why we restrict to a  single IFS test at any time)
>> If 4 happens while 1 or 2 is in progress , we return busy.  My earlier explanation was trying to explain why we are preventing 4 when 1 or 2   is in progress.
>>
>> As to the question of why would a user do 4 while 1 or 2 is in progress, there is no reason for him to do that, both the run_test (percpu and global) are blocking.
>> But if he issues a test from one shell and uses another shell to query the status  (while it is still in progress) he will see busy.
> 
> ...and what about the race that the semaphore cannot solve of test run
> launch racing collection of previous run results?


pardon me if I am missing something obvious here … but everybody (the 4 scenarios listed above) tries to acquire the same semaphore, or returns -EBUSY if try_lock() fails.
So in case of triggering "run_test" and viewing "status" racing scenario you mention, the below are the 2 interleaving I see

if "echo 1 >  /sys/devices/sysem/cpu/cpu10/ifs/run_test" gets the lock , the parallel  "cat  /sys/devices/sysem/cpu/cpu10/ifs/status" will return busy (and not the previous status)
if  "cat  /sys/devices/sysem/cpu/cpu10/ifs/status", gets  the lock it will display the status of the last test result and  the parallel  "echo 1 >  /sys/devices/sysem/cpu/cpu10/ifs/run_test" will fail with busy

This I believe is consistent behavior.