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

Kui-Feng Lee
2024-05-23T19:03:40Z

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.

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

>
>>
>>>>   };
>>>
>