Re: [PATCH bpf-next v4 3/7] bpf: support epoll from bpf struct_ops links.

[Martin KaFai Lau <martin.lau@linux.dev>]

On 5/23/24 12:03 PM, Kui-Feng Lee wrote:
> 
> 
> On 5/23/24 11:34, Martin KaFai Lau wrote:
>> On 5/23/24 11:24 AM, Kui-Feng Lee wrote:
>>>
>>>
>>> On 5/23/24 10:23, Martin KaFai Lau wrote:
>>>> On 5/21/24 3:51 PM, Kui-Feng Lee wrote:
>>>>> +static __poll_t bpf_link_poll(struct file *file, struct poll_table_struct 
>>>>> *pts)
>>>>> +{
>>>>> +    struct bpf_link *link = file->private_data;
>>>>> +
>>>>> +    if (link->ops->poll)
>>>>> +        return link->ops->poll(file, pts);
>>>>> +
>>>>> +    return 0;
>>>>
>>>> The current bpf_link_fops.poll is NULL before this patch. From vfs_poll, it 
>>>> seems to be DEFAULT_POLLMASK for this case. Please double check.
>>>
>>>
>>> Yes, it returns DEFAULT_POLLMASK if file->f_op->epoll is NULL. But,
>>> before this patch, link can not be added to an epoll. See the
>>> explanation below.
>>
>> How about select() and poll() that do not need epoll_ctl() setup?
> 
> AFAIK, they just don't check it at all, calling vfs_poll() directly.

right, vfs_poll returns DEFAULT_POLLMASK which is not 0.

#define DEFAULT_POLLMASK (EPOLLIN | EPOLLOUT | EPOLLRDNORM | EPOLLWRNORM)

static inline __poll_t vfs_poll(struct file *file, struct poll_table_struct *pt)
{
	if (unlikely(!file->f_op->poll))
		return DEFAULT_POLLMASK;
	return file->f_op->poll(file, pt);
}

but this discussion is moot if another file_operations instance is used.

> 
>>
>>>
>>>>
>>>>> +}
>>>>> +
>>>>>   static const struct file_operations bpf_link_fops = {
>>>>>   #ifdef CONFIG_PROC_FS
>>>>>       .show_fdinfo    = bpf_link_show_fdinfo,
>>>>> @@ -3157,6 +3167,7 @@ static const struct file_operations bpf_link_fops = {
>>>>>       .release    = bpf_link_release,
>>>>>       .read        = bpf_dummy_read,
>>>>>       .write        = bpf_dummy_write,
>>>>> +    .poll        = bpf_link_poll,
>>>>
>>>> Same here. What does the epoll_ctl(EPOLL_CTL_ADD) currently expect for link 
>>>> (e.g. cgroup) that does not support poll?
>>>>
>>>
>>> epoll_ctl() always returns -EPERM for files not supporting poll.
>>> Should I add another instance of struct file_operations to keep the
>>> consistency for other types of links?
>>
>> imo, it makes sense to have another instance for link that supports poll such 
>> that epoll_ctl(EPOLL_CTL_ADD) can fail early for the unsupported links.
> 
> Ok! I will add another instance.