Message-ID: <CADXeF1HQH=bhNWAtafUKD3-WoHhQEB-=TpwN9GdOOZ9PSaLktQ@mail.gmail.com>
Date: Thu, 14 Nov 2024 14:58:18 +0900
From: Yuyang Huang <yuyanghuang@...gle.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: Hangbin Liu <liuhangbin@...il.com>, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Simon Horman <horms@...nel.org>, David Ahern <dsahern@...nel.org>, roopa@...ulusnetworks.com, 
	jiri@...nulli.us, stephen@...workplumber.org, netdev@...r.kernel.org, 
	Maciej Żenczykowski <maze@...gle.com>, 
	Lorenzo Colitti <lorenzo@...gle.com>
Subject: Re: [PATCH net-next] netlink: add igmp join/leave notifications

> So that probably leads to a NACK for these patches. If APF is your
> target, and to me it seems unlikely APF will get accepted into
> mainline, there is no need for these netlink changes. Hence a NACK.

> And this is probably your way in. Forget about APF when talking to
> mainline.

Thanks for the advice. I'll remove the APF context from the commit
message, as these patches should benefit mainline regardless of their
APF use case.

> Show these patches are useful in general. Explain the use
> case of IGMP/MLD control in user space using existing APIs to control
> the network stack and hardware.

> Yes, this is going in correct direction to get these patches
> merged. Focus on this. Solve these problems with ip monitor etc.  Once
> merged you can then use it with the out of mainline APF.

I plan to include MLD modifications and rewrite the commit message as
follows in patch v2.  I will also send the ip monitor patches in
parallel.

Please let me know if you have any further suggestions.

```
netlink: add IGMP/MLD join/leave notifications

This change introduces netlink notifications for multicast address
changes. The following features are included:
* Addition and deletion of multicast addresses are reported using
  RTM_NEWMULTICAST and RTM_DELMULTICAST messages with AF_INET and
  AF_INET6.
* Two new notification groups: RTNLGRP_IPV4_MCADDR and
  RTNLGRP_IPV6_MCADDR are introduced for receiving these events.

This change allows user space applications (e.g., ip monitor) to
efficiently track multicast group memberships by listening for netlink
events. Previously, applications relied on inefficient polling of
procfs, introducing delays. With netlink notifications, applications
receive realtime updates on multicast group membership changes,
enabling more precise metrics collection and system monitoring.

This change also empowers user space applications to manage multicast
filters and IGMP/MLD offload rules using the same netlink notification
mechanism. This allows applications to dynamically adjust rules and
configurations via generic netlink communication with the Wi-Fi driver,
offering greater flexibility and updatability compared to implementing
all logic within the driver itself. This is a key consideration for some
commercial devices.
```

Thanks,
Yuyang

On Thu, Nov 14, 2024 at 6:06 AM Andrew Lunn <andrew@...n.ch> wrote:
>
> On Wed, Nov 13, 2024 at 01:26:29PM +0900, Yuyang Huang wrote:
> > >O.K. WiFi is not my area. But i'm more interested in uAPIs, and
> > > ensuring you are not adding APIs which promote kernel bypass.
>
> [Off list]
>
> >
> > WiFi chipset vendors must implement the Android WiFi HAL to install
> > and read the APF program from WiFi firmware. The Android System Server
> > will talk to vendor HAL service using the WiFi HAL. The datapath is:
> > Network Stack process -> Android System Server -> vendor HAL service
> > -> WiFi driver -> WiFi firmware. The Android WiFi HAL is specific to
> > Android. The vendor HAL service, WiFi driver and WiFi firmware are all
> > vendor proprietary software.  In other words, those API are not in
> > mainline yet.
>
> So that probably leads to a NACK for these patches. If APF is your
> target, and to me it seems unlikely APF will get accepted into
> mainline, there is no need for these netlink changes. Hence a NACK.
>
> > >Do the new netlink message make sense without APF? Can i write a user
> > >space IGMP snooping implementation and then call bridge mdb
> > >add/del/replace?
> >
> > The RTM_NEWMULTICAST and RTM_DELMULTICAST events introduced in this
> > patch enable user space implementation of IGMP/MLD offloading and
> > IPv4/IPv6 multicast filtering. I have limited knowledge on how to
> > implement IGMP snooping correctly so I don't know if they are
> > sufficient.
> >
> > These two events have broader applications beyond APF.
>
> And this is probably your way in. Forget about APF when talking to
> mainline. Show these patches are useful in general. Explain the use
> case of IGMP/MLD control in user space using existing APIs to control
> the network stack and hardware.
>
> > It might also make sense to consider whether to accept the proposed
> > APIs from an API completeness perspective. The current netlink API for
> > multicast addresses seems incomplete. While RTM_GETMULTICAST exists,
> > it only supports IPv6, not IPv4. This limitation forces tools like 'ip
> > maddr' to rely on parsing procfs instead of using netlink.
> > Additionally, 'ip monitor' cannot track multicast address additions or
> > removals. I feel it would make sense to have full netlink based
> > dumping and event notification support for both IPv4/IPv6 multicast
> > addresses as well.
>
> Yes, this is going in correct direction to get these patches
> merged. Focus on this. Solve these problems with ip monitor etc.  Once
> merged you can then use it with the out of mainline APF.
>
>         Andrew

Powered by blists - more mailing lists