Re: [PATCH 01/11] netdev: Add a wdev_id based frame watch API

[Denis Kenzior <denkenz@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3430 bytes --]

Hi Andrew,

On 10/22/19 6:56 PM, Andrew Zaborowski wrote:
> On Tue, 22 Oct 2019 at 16:53, Denis Kenzior <denkenz@gmail.com> wrote:
>> On 10/21/19 8:55 AM, Andrew Zaborowski wrote:
>>> diff --git a/src/netdev.h b/src/netdev.h
>>> index 114a6035..624811b2 100644
>>> --- a/src/netdev.h
>>> +++ b/src/netdev.h
>>> @@ -197,6 +197,12 @@ uint32_t netdev_frame_watch_add(struct netdev *netdev, uint16_t frame_type,
>>>                                void *user_data);
>>>    bool netdev_frame_watch_remove(struct netdev *netdev, uint32_t id);
>>>
>>> +uint32_t netdev_wdev_frame_watch_add(uint64_t wdev_id, uint16_t frame_type,
>>> +                             const uint8_t *prefix, size_t prefix_len,
>>> +                             netdev_frame_watch_func_t handler,
>>> +                             void *user_data);
>>> +bool netdev_wdev_frame_watch_remove(uint32_t id);
>>> +
>>
>> So this really seems like it is jammed in there and it doesn't fit well.
>>
>> I think that we should make frame watches into a separate module now
>> that they no longer are only associated with a netdev.
> 
> Ok, I only put it inside netdev as an extension of the current api.

I know.  But given that this isn't urgent, I'd rather do it right the 
first time.

> 
>> This would allow
>> us to also track iftype changes and wipe out those frame watches that
>> are no longer registered with the kernel.  And also allow us not to try
>> and register frame watches that are already live (and bounce with an
>> -EAGAIN from the kernel).
> 
> The problem is how we will know what logic is implemented in the
> running kernel version.  We also watch iftype changes inside netdev so

Well, the particular fix from me got backported into every single LTS 
kernel.  So we just assume the fix is included.  Unless you want to go 
full wpa_s and use a separate socket for each frame watch.

> we can more comfortably do it here but I'm not sure it helps us.  In
> P2P I'm registering and unregistering the watches for specific
> operations and we never switch iftypes.  I think we should either have

Yeah, I think you have to bite the bullet and implement a separate 
socket for these.

> the kernel have a simple frame unregister command or keep all of the

Well, we tried that and got rejected.  See [1].  So as I said, until the 
kernel guys start treating nl80211 as a proper API, we have to live with 
this.

> watches for simplicity.

I don't think this is a good idea.  It will cause unnecessary wakeups 
and round trips into the kernel.  And any one of these is one too many.

> 
>>
>> We could also implement 'transient' frame watch sets by using a genl
>> socket dedicated to each set.  At least until the kernel people fix
>> their subsystem.
> 
> This sounds like it's much more costly in terms of syscalls and kernel
> work (and code) than occasionally getting the EAGAIN for a register
> command.

If you assume that frame watches are purged on iftype change, then most 
of the EAGAINs are only caused by netdev ifup/ifdown.  So those we can 
take care of easily.

Unnecessary wakeups should be avoided completely.  So if you have p2p 
frame watches and you can group them into a set to enable / disable, 
then they should be registered for on a separate socket.

> 
> Best regards
> 

Regards,
-Denis

[1] https://patchwork.kernel.org/patch/11131097/