Date:	Thu, 11 Dec 2014 18:47:36 +0000
From:	"Arad, Ronen" <ronen.arad@...el.com>
To:	Hubert Sokolowski <h.sokolowski@....edu.pl>,
	Roopa Prabhu <roopa@...ulusnetworks.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:	Jamal Hadi Salim <hadi@...atatu.com>
Subject: RE: [PATCH net-next RESEND] net: Do not call ndo_dflt_fdb_dump if
 ndo_fdb_dump is defined.



> -----Original Message-----
> From: netdev-owner@...r.kernel.org [mailto:netdev-
> owner@...r.kernel.org] On Behalf Of Hubert Sokolowski
> Sent: Thursday, December 11, 2014 8:40 AM
> To: Roopa Prabhu
> Cc: netdev@...r.kernel.org; Jamal Hadi Salim
> Subject: Re: [PATCH net-next RESEND] net: Do not call ndo_dflt_fdb_dump if
> ndo_fdb_dump is defined.
> 
> > I think commit
> > "5e6d243587990a588143b9da3974833649595587 "bridge: netlink dump
> > interface at par with brctl" tried to make sure even the dflt entries
> > (ie dev->uc and dev->mc) were also printed in the below case. ie the
> > 'self' entries in the below output.
> >
> > # ./bridge fdb show brport eth1
> > 02:00:00:12:01:02 vlan 0 master br0 permanent
> > 00:17:42:8a:b4:05 vlan 0 master br0 permanent
> > 00:17:42:8a:b4:07 self permanent
> > 33:33:00:00:00:01 self permanent
> >
> > Am guessing reverting the patch is going to make the 'self' entries in
> > the above output to go away.
> > Can you please confirm ?.
> 
> I don't want you to revert the patch, as the main goal of the patch was to
> enable filtering in the kernel. I am only proposing to revert part of it that
> allows driver to implement own dump.
> This does not break the filtering in the kernel.
> Whether the 'self' entries will go away it depends if the driver overrides
> ndo_fdb_dump callback with its own. For cases where the driver does not
> implement the callback, the dflt callback is still called showing 'self' entries:
> [root@...pixa00378825 ~]# bridge fdb show
> 33:33:00:00:00:01 dev em1 self permanent
> 01:00:5e:00:00:01 dev em1 self permanent
> 33:33:00:00:00:01 dev p4p1 self permanent
> 01:00:5e:00:00:01 dev p4p1 self permanent 33:33:ff:81:56:db dev p4p1 self
> permanent 01:00:5e:00:00:fb dev p4p1 self permanent
> 33:33:00:00:00:01 dev dummy0 self permanent
> 
> >
> > Also, if i hear your concern correctly, for bridge ports that
> > implement ndo_fdb_dump, with commit
> > 5e6d243587990a588143b9da3974833649595587, we will get two entries
> for each 'self' entry above.
> > Can you also paste sample output for that ?.
> 
> My patch affects *only* drivers that implements own dump callback.
> Implementing own dump callback means the driver want to have a control
> over what is being dumped. For example you may want to dump a hardware
> MAC table only (my case) where 'self' entries created by kernel make no
> sense.
> Also there are drivers that calls dflt callback from inside own dump function.
> Please see following dump callback implemented for QLogic:
> static int qlcnic_fdb_dump(struct sk_buff *skb, struct netlink_callback *ncb,
>                         struct net_device *netdev,
>                         struct net_device *filter_dev, int idx) {
>         struct qlcnic_adapter *adapter = netdev_priv(netdev);
> 
>         if (!adapter->fdb_mac_learn)
>                 return ndo_dflt_fdb_dump(skb, ncb, netdev, filter_dev, idx); [...]
> 
> Another example of dflt being called twice is macvlan.c where
> ndo_fdb_dump is actually initialized with the dflt callback:
> macvlan.c:1022:        .ndo_fdb_dump           = ndo_dflt_fdb_dump,
> 
> Thanks,
> Hubert
>

I agree with Hubert on that. When a device defines its own ndo_fdb_dump it implies it wants control over the information that will be returned to user-space.
The proposed patch changes the recent status quo but it only restores it to the way it was before 5e6d243587990a588143b9da3974833649595587.
A compromise could be to also include in the same patch-set patches to the drivers that have benefited from the implicit call to ndo_dflt_fdb_dump.

-Ronen
 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in the body
> of a message to majordomo@...r.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists