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

[Kui-Feng Lee <sinquersw@gmail.com>]

On 5/23/24 12:10, Martin KaFai Lau wrote:
> 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.

Sure! I am adding another instance.

> 
>>
>>>
>>>>
>>>>>
>>>>>> +}
>>>>>> +
>>>>>>   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.
>