Re: [PATCH 07/17] drm/xe/oa/uapi: Define and parse OA stream properties

[Lionel Landwerlin <lionel.g.landwerlin@intel.com>]

On 19/12/2023 18:40, Umesh Nerlige Ramappa wrote:
> On Tue, Dec 19, 2023 at 06:29:56PM +0200, Lionel Landwerlin wrote:
>> On 19/12/2023 18:26, Umesh Nerlige Ramappa wrote:
>>> On Mon, Dec 18, 2023 at 06:59:57PM -0800, Dixit, Ashutosh wrote:
>>>> On Thu, 07 Dec 2023 22:43:19 -0800, Ashutosh Dixit wrote:
>>>>>
>>>>> +    /**
>>>>> +     * @DRM_XE_OA_PROPERTY_OPEN_FLAGS: CLOEXEC and NONBLOCK flags 
>>>>> are
>>>>> +     * directly applied to returned OA fd. DISABLED opens the OA 
>>>>> stream in a
>>>>> +     * DISABLED state (see @DRM_XE_PERF_IOCTL_ENABLE).
>>>>> +     */
>>>>> +    DRM_XE_OA_PROPERTY_OPEN_FLAGS,
>>>>> +#define DRM_XE_OA_FLAG_FD_CLOEXEC    (1 << 0)
>>>>> +#define DRM_XE_OA_FLAG_FD_NONBLOCK    (1 << 1)
>>>>> +#define DRM_XE_OA_FLAG_DISABLED        (1 << 2)
>>>>
>>>> I am wondering why these flags should be part of this uapi:
>>>>
>>>> * O_CLOEXEC and O_NONBLOCK can be set on the returned stream fd 
>>>> using fcntl
>>>>  (see man 2 fcntl)
>>>
>>> I think the O_CLOEXEC was used so that a fork doesn't carry over the 
>>> fd to the child. For the OA use case, we want to prevent that.  
>>> However, these flags don't really need to be passed separately. They 
>>> can be flags in the stream open property.
>>>
>>> Umesh
>>>
>>
>> You know that the application can set those flags by using the 
>> fcntl() syscall?
>>
>> It doesn't look like it's a useful feature to add in the driver.
>>
>
> Right. It does look like it's not needed in the driver.
>
> I just don't know if there was a reason to include it in the same call 
> as stream open ioctl. My guess is that we didn't want them to be 
> separate calls due the nature of OA use case - privileged and single 
> user. The application could just open a stream fd and fork a bunch of 
> threads and all threads would have access to the stream fd (even if 
> they drop root?).
> Or I might be overthinking this. Maybe it's just there in the driver 
> because fcntl mentions some races that may/may not apply to our use 
> case. In practice, the application will likely call the fcntl right 
> away and since OA does not support multiple users, the above concerns 
> are not relevant, so fine to do it in fcntl.


Yeah I think it's likely userspace driver code that will call fcntl 
right after OA open. So it doesn't look like the KMD needs to also 
handle that with additional flags.


-Lionel


>
> Thanks,
> Umesh
>>
>> -Lionel
>>
>>
>>>> * DRM_XE_OA_FLAG_DISABLED can just be a stream open property, 
>>>> doesn't need
>>>>  to be a fd flag.
>>>>
>>>> Comments?
>>>
>>>
>>>>
>>>> Thanks.
>>>> -- 
>>>> Ashutosh
>>
>>