Message-ID: <a7629503af0aa345d65b1651586f906dc7259484.camel@vyatta.att-mail.com>
Date:   Tue, 18 Sep 2018 14:12:48 +0100
From:   Patrick Ruddy <pruddy@...tta.att-mail.com>
To:     Roopa Prabhu <roopa@...ulusnetworks.com>
Cc:     netdev <netdev@...r.kernel.org>,
        Jiří Pírko <jiri@...nulli.us>,
        Stephen Hemminger <stephen@...workplumber.org>,
        Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
Subject: Re: [PATCH net-next v3 1/2] netlink: ipv4 igmp join notifications

On Thu, 2018-09-13 at 10:03 -0700, Roopa Prabhu wrote:
> On Thu, Sep 6, 2018 at 8:40 PM, Roopa Prabhu <roopa@...ulusnetworks.com> wrote:
> > On Thu, Sep 6, 2018 at 2:10 AM, Patrick Ruddy
> > <pruddy@...tta.att-mail.com> wrote:
> > > Some userspace applications need to know about IGMP joins from the
> > > kernel for 2 reasons:
> > > 1. To allow the programming of multicast MAC filters in hardware
> > > 2. To form a multicast FORUS list for non link-local multicast
> > >    groups to be sent to the kernel and from there to the interested
> > >    party.
> > > (1) can be fulfilled but simply sending the hardware multicast MAC
> > > address to be programmed but (2) requires the L3 address to be sent
> > > since this cannot be constructed from the MAC address whereas the
> > > reverse translation is a standard library function.
> > > 
> > > This commit provides addition and deletion of multicast addresses
> > > using the RTM_NEWMDB and RTM_DELMDB messages with AF_INET. It also
> > > provides the RTM_GETMDB extension to allow multicast join state to
> > > be read from the kernel.
> > > 
> > > Signed-off-by: Patrick Ruddy <pruddy@...tta.att-mail.com>
> > > ---
> > > v3 rework to use RTM_***MDB messages as per review comments.
> > 
> > Patrick, this version seems to be using RTM_***MDB msgs with the
> > RTM_*ADDR format.
> > We cant do that...because existing RTM_MDB users will be confused.
> > 
> > My request was to evaluate RTM_***MDB msg format. see
> > nlmsg_populate_mdb_fill for details.
> > 
> > If you can wait a day or two I can share some experimental code that
> > moves high level RTM_*MDB msg handling into net/core/rtnetlink.c
> > similar to RTM_*FDB
> > 
> 
> I was trying to get a default per interface (non bridge) RTM_*MDB
> working, but realized that the dev->mc
> entries are already getting dumped as part of RTM_*FDB msgs instead of
> RTM_*MDB. (see net/core/rtnetlink.c:ndo_dflt_fdb_dump).
> This adds another wrench.
> 
> so, that puts us back to your use of RTM_NEWADDR.
> Instead of using IFA_ADDRESS, you could introduce a new one
> IFA_IGMP_MULTICAST  (since IFA_MULTICAST is already taken).
> 
> 
> To keep existing users of RTM_NEWADDR unaffected. I think you can use
> the IPMR family with RTM_NEWADDR.
> We can introduce new notification group. (We can choose to add a new
> family too, but that seems unnecessary)
> 
> since you only need dumps:
> rtnl_register(RTNL_FAMILY_IPMR, RTM_GETADDR, NULL, igmp_rtm_dumpaddrs, 0);
> 
> For notifications, since we already have many variants for routes, I
> don't see a problem adding similar addr variants
> RTNLGRP_IPV4_MCADDR
> 
> (Others on the list may have more feedback).

I've hit a small snag with adding the new groups. The number of defined
groups currently sits at 31 so I can only add one before hitting the
limit defined by the 32 bit groups bitmask in socakddr_nl. I can use 1
group for both v4 and v6 notifications which seems like the sensible
options since the AF is carried separately, but it breaks the precedent
where there are separate IPV4 and IPV6 groups for IFADDR.

I have the combined group patches ready and can share them if that's
the preference.

Has there been any previous discussion about extending the number of
availabel groups?

Powered by blists - more mailing lists