netdev - Re: [PATCH 1/3] net: add a new NETDEV

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1377016516.13829.17.camel@jlt4.sipsolutions.net>
Date:	Tue, 20 Aug 2013 18:35:16 +0200
From:	Johannes Berg <johannes@...solutions.net>
To:	Florian Fainelli <f.fainelli@...il.com>
Cc:	netdev <netdev@...r.kernel.org>, amwang <amwang@...hat.com>,
	Jiri Pirko <jiri@...nulli.us>,
	Stephen Hemminger <stephen@...workplumber.org>,
	kaber <kaber@...sh.net>, David Miller <davem@...emloft.net>,
	vyasevic <vyasevic@...hat.com>,
	Eric Dumazet <eric.dumazet@...il.com>
Subject: Re: [PATCH 1/3] net: add a new NETDEV_CHANGEROOM event type

On Tue, 2013-08-20 at 16:30 +0100, Florian Fainelli wrote:

> > Also, maybe it would be worth doing it in one call? If you need to
> > change both, then you'd end up calling the notifier twice, which is less
> > efficient?
> 
> I have mixed feelings about this. I do not expect changing the
> headroom/tailroom to be in a hot-path, and we would need to have a
> name such as dev_set_head_and_tailroom() or something that clearly
> states that it operates on both quantities. Looking at the subsystems
> and drivers, there are quite a lot of users which only set one or the
> other, occasionaly both before registration.

No, it shouldn't be on a path that has any performance impact at all,
that's true.

> > I suppose you could make them 'int' arguments and reserve -1
> > for no changes, or just require both new values to be given (if doing
> > this at all.)
> 
> What I like about keeping them separate is that we can use the
> "native" storage type that is used in struct net_device, and have
> compile-time checking of this.

Makes sense.

I was really more thinking about the notifier complexity.

Right now, you can potentially blow up your iterations - for example if
you have a vlan on a bridge:
 * driver sets headroom (or tailroom)
 * this iterates all netdevs, including the bridge
 * bridge calls the function again, and while iterating iterates again,
then
   going into the vlan
   (is it even valid to iterate while iterating?)
 * vlan calls it again and it iterates again, doing nothing this time

So now you've iterated the netdevs many times...

johannes

--
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