From: Pavel Emelyanov <hidden> Date: 2012-07-30 04:35:07
Currently the RTM_NEWLINK results in -EOPNOTSUPP if the ifinfomsg->ifi_index
is not zero. I propose to allow requesting ifindices on link creation. This
is required by the checkpoint-restore to correctly restore a net namespace
(i.e. -- a container). The question what to do with pre-created devices such
as lo or sit fbdev is open, but for manually created devices this can be
solved by this patch.
Signed-off-by: Pavel Emelyanov <redacted>
---
From: Pavel Emelyanov <hidden> Date: 2012-07-30 04:37:02
The ifinfomsg is in there (thanks kaber@ for foreseeing this long time ago),
so take the given ifidex and register netdev with it.
Signed-off-by: Pavel Emelyanov <redacted>
---
From: Eric W. Biederman <hidden> Date: 2012-07-30 10:50:07
Pavel Emelyanov [off-list ref] writes:
Currently the RTM_NEWLINK results in -EOPNOTSUPP if the ifinfomsg->ifi_index
is not zero. I propose to allow requesting ifindices on link creation. This
is required by the checkpoint-restore to correctly restore a net namespace
(i.e. -- a container). The question what to do with pre-created devices such
as lo or sit fbdev is open, but for manually created devices this can be
solved by this patch.
Have you walked through and found the locations where we still rely on
ifindex being globally unique?
Last time I was working in this area there were serveral places where
things were indexed by just the interface index.
I susepct it might be easier to generate hotplug events at restart time
saying someone removed and added an identical set of network devices.
Certainly for physical hardware that needs to happen, because things
like mac addresses will change.
Eric
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Eric W. Biederman <hidden> Date: 2012-07-30 10:56:53
ebiederm@xmission.com (Eric W. Biederman) writes:
Pavel Emelyanov [off-list ref] writes:
quoted
Currently the RTM_NEWLINK results in -EOPNOTSUPP if the ifinfomsg->ifi_index
is not zero. I propose to allow requesting ifindices on link creation. This
is required by the checkpoint-restore to correctly restore a net namespace
(i.e. -- a container). The question what to do with pre-created devices such
as lo or sit fbdev is open, but for manually created devices this can be
solved by this patch.
Have you walked through and found the locations where we still rely on
ifindex being globally unique?
Last time I was working in this area there were serveral places where
things were indexed by just the interface index.
If it is really safe to make ifindex per network namespace at this
point you can make dev_new_ifindex have a per network namespace base
counter, and that will fix your problems with the loopback device.
Unless you have done the work to root out the last of dependencies on
ifindex being globally unique I think you will run into some operational
problems.
Eric
From: Eric Dumazet <hidden> Date: 2012-07-30 11:51:07
On Mon, 2012-07-30 at 03:49 -0700, Eric W. Biederman wrote:
Pavel Emelyanov [off-list ref] writes:
quoted
Currently the RTM_NEWLINK results in -EOPNOTSUPP if the ifinfomsg->ifi_index
is not zero. I propose to allow requesting ifindices on link creation. This
is required by the checkpoint-restore to correctly restore a net namespace
(i.e. -- a container). The question what to do with pre-created devices such
as lo or sit fbdev is open, but for manually created devices this can be
solved by this patch.
Have you walked through and found the locations where we still rely on
ifindex being globally unique?
Last time I was working in this area there were serveral places where
things were indexed by just the interface index.
Really ? This would be very strange.
AFAIK dev_new_index() is always called, even in the
dev_change_net_namespace() case if there is a conflict.
And dev_new_index() could use a pernet net->ifindex instead of a
shared/static one.
From: Eric W. Biederman <hidden> Date: 2012-07-30 12:33:14
Eric Dumazet [off-list ref] writes:
On Mon, 2012-07-30 at 03:49 -0700, Eric W. Biederman wrote:
quoted
Pavel Emelyanov [off-list ref] writes:
quoted
Currently the RTM_NEWLINK results in -EOPNOTSUPP if the ifinfomsg->ifi_index
is not zero. I propose to allow requesting ifindices on link creation. This
is required by the checkpoint-restore to correctly restore a net namespace
(i.e. -- a container). The question what to do with pre-created devices such
as lo or sit fbdev is open, but for manually created devices this can be
solved by this patch.
Have you walked through and found the locations where we still rely on
ifindex being globally unique?
Last time I was working in this area there were serveral places where
things were indexed by just the interface index.
Really ? This would be very strange.
There at least were places that used oif or iff without being pernet
last time I was working on this.
It was never code that I understood particularly well so my memory of
what that code is, is unfortunately fuzzy.
AFAIK dev_new_index() is always called, even in the
dev_change_net_namespace() case if there is a conflict.
Except we never have a conflict because it takes an absurd number of
network devices to cause a 32bit counter to wrap.
And dev_new_index() could use a pernet net->ifindex instead of a
shared/static one.
Yes. I made all of the core changes, and held back on making
dev_new_index() use a pernet net->ifindex because of a couple of problem
cases.
It has been a long time and those cases might have been fixed.
I'm not seeing anything obvious in the network stack with a quick skim,
but before we start relying on the property that interface indicies are
not globally unique I expect an good hard look at the networking stack
to see if any of those cases where there were problems still exist.
Eric
From: Pavel Emelyanov <hidden> Date: 2012-07-31 09:03:46
On 07/30/2012 02:56 PM, Eric W. Biederman wrote:
ebiederm@xmission.com (Eric W. Biederman) writes:
quoted
Pavel Emelyanov [off-list ref] writes:
quoted
Currently the RTM_NEWLINK results in -EOPNOTSUPP if the ifinfomsg->ifi_index
is not zero. I propose to allow requesting ifindices on link creation. This
is required by the checkpoint-restore to correctly restore a net namespace
(i.e. -- a container). The question what to do with pre-created devices such
as lo or sit fbdev is open, but for manually created devices this can be
solved by this patch.
Have you walked through and found the locations where we still rely on
ifindex being globally unique?
Last time I was working in this area there were serveral places where
things were indexed by just the interface index.
If it is really safe to make ifindex per network namespace at this
point you can make dev_new_ifindex have a per network namespace base
counter, and that will fix your problems with the loopback device.
Not it's not so unfortunately :(
First, let's imagine that on host A the loopback device got registered as
first device, but on host B for some reason some other device got registered
first. In that case after migration from A to B the lo on B will have index
equals 2. And there's no any strict requirement that lo's per net operations
are registered first. Please, correct me if I'm wrong.
Next. In fact, lo is not the only problem. Look at the e.g. sit versus ipgre
fallback devices. Both gets created on netns creation and obtain whatever
ifindices are generated for them. Even if we make ifidex per netns chances
that sit gets registered _strictly_ before ipgre equal zero, since they are
both modules.
Unless you have done the work to root out the last of dependencies on
ifindex being globally unique I think you will run into some operational
problems.
I totally agree with that. Before doing this patch I revisited the ancient
attempt to make ifindices per netns and checked the issues Dave and you
discussed then -- I have looked through how the ifindices are used in the
networking code and found no places where the system-wide uniqueness is still
required. That's why I proposed this patch for inclusion. If you know the
places I've missed, please let me know, I will work on it.
From: Pavel Emelyanov <hidden> Date: 2012-07-31 09:06:43
I'm not seeing anything obvious in the network stack with a quick skim,
but before we start relying on the property that interface indicies are
not globally unique I expect an good hard look at the networking stack
to see if any of those cases where there were problems still exist.
Just an idea -- is it worth moving the possibility to have ifindidces intersect
under CONFIG_<SOMETHING> (EXPERT/CHECKPOINT_RESTORE) to let wider audience check
the code in real-life?
From: Eric W. Biederman <hidden> Date: 2012-07-31 11:58:48
Pavel Emelyanov [off-list ref] writes:
On 07/30/2012 02:56 PM, Eric W. Biederman wrote:
quoted
ebiederm@xmission.com (Eric W. Biederman) writes:
quoted
Pavel Emelyanov [off-list ref] writes:
quoted
Currently the RTM_NEWLINK results in -EOPNOTSUPP if the ifinfomsg->ifi_index
is not zero. I propose to allow requesting ifindices on link creation. This
is required by the checkpoint-restore to correctly restore a net namespace
(i.e. -- a container). The question what to do with pre-created devices such
as lo or sit fbdev is open, but for manually created devices this can be
solved by this patch.
Have you walked through and found the locations where we still rely on
ifindex being globally unique?
Last time I was working in this area there were serveral places where
things were indexed by just the interface index.
If it is really safe to make ifindex per network namespace at this
point you can make dev_new_ifindex have a per network namespace base
counter, and that will fix your problems with the loopback device.
Not it's not so unfortunately :(
First, let's imagine that on host A the loopback device got registered as
first device, but on host B for some reason some other device got registered
first. In that case after migration from A to B the lo on B will have index
equals 2. And there's no any strict requirement that lo's per net operations
are registered first. Please, correct me if I'm wrong.
Actually there is a hard requirement that the loopback device be the
last device in a network namespace to be unregistered. We meet that
requirement by registering the loopback device first
"net/core/dev.c:net_dev_init()".
Next. In fact, lo is not the only problem. Look at the e.g. sit versus ipgre
fallback devices. Both gets created on netns creation and obtain whatever
ifindices are generated for them. Even if we make ifidex per netns chances
that sit gets registered _strictly_ before ipgre equal zero, since they are
both modules.
True. However those fallback devices should no longer be needed,
and even if they are I think you can delete and recreate them.
Making lo the particularly interesting case.
quoted
Unless you have done the work to root out the last of dependencies on
ifindex being globally unique I think you will run into some operational
problems.
I totally agree with that. Before doing this patch I revisited the ancient
attempt to make ifindices per netns and checked the issues Dave and you
discussed then -- I have looked through how the ifindices are used in the
networking code and found no places where the system-wide uniqueness is still
required. That's why I proposed this patch for inclusion. If you know the
places I've missed, please let me know, I will work on it.
I took a quick look and I did not see anything. I saw places under
net/sched/ that looked a bit suspicious, and of course there are places
where we use oif and iff in some of the routing code that make we wonder
a bit. But if you have looked and if I have looked I think we are ok.
Just an idea -- is it worth moving the possibility to have ifindidces intersect
under CONFIG_<SOMETHING> (EXPERT/CHECKPOINT_RESTORE) to let wider audience check
the code in real-life?
I think the best testing we are going to get diversity wise is to create
a per netns counter into dev_new_index when net-next opens up.
Having an ifindex that we can only set at netdevice creation time seems
reasonable.
Eric
From: Pavel Emelyanov <hidden> Date: 2012-07-31 13:30:26
quoted
First, let's imagine that on host A the loopback device got registered as
first device, but on host B for some reason some other device got registered
first. In that case after migration from A to B the lo on B will have index
equals 2. And there's no any strict requirement that lo's per net operations
are registered first. Please, correct me if I'm wrong.
Actually there is a hard requirement that the loopback device be the
last device in a network namespace to be unregistered. We meet that
requirement by registering the loopback device first
"net/core/dev.c:net_dev_init()".
Hm... Indeed, and this is good news!
quoted
Next. In fact, lo is not the only problem. Look at the e.g. sit versus ipgre
fallback devices. Both gets created on netns creation and obtain whatever
ifindices are generated for them. Even if we make ifidex per netns chances
that sit gets registered _strictly_ before ipgre equal zero, since they are
both modules.
True. However those fallback devices should no longer be needed,
and even if they are I think you can delete and recreate them.
Good idea! I will look at that direction.
Making lo the particularly interesting case.
Yup, provided we can manually recreate those auto-created devices this solves
the issue.
quoted
Just an idea -- is it worth moving the possibility to have ifindidces intersect
under CONFIG_<SOMETHING> (EXPERT/CHECKPOINT_RESTORE) to let wider audience check
the code in real-life?
I think the best testing we are going to get diversity wise is to create
a per netns counter into dev_new_index when net-next opens up.
Having an ifindex that we can only set at netdevice creation time seems
reasonable.
From: Eric Dumazet <hidden> Date: 2012-08-02 10:28:35
On Tue, 2012-07-31 at 04:58 -0700, Eric W. Biederman wrote:
Making lo the particularly interesting case.
BTW, I noticed in my benchmarks, that once I remove the contention on
dst refcnt (using a percpu cache of dsts), I have a strange performance
cost accessing net->loopback_dev->ifindex in ip_route_output_key.
Strange because I see no false sharing on this ifindex location for
loopback device.
So we probably can save some cycles adding a net->loopback_ifindex
to remove one dereference.
If ifindex are per network space, I guess we'll need to change
arp_hashfn() or else we'll use some slots more than others.
From: Eric W. Biederman <hidden> Date: 2012-08-02 11:10:08
Eric Dumazet [off-list ref] writes:
On Tue, 2012-07-31 at 04:58 -0700, Eric W. Biederman wrote:
quoted
Making lo the particularly interesting case.
BTW, I noticed in my benchmarks, that once I remove the contention on
dst refcnt (using a percpu cache of dsts), I have a strange performance
cost accessing net->loopback_dev->ifindex in ip_route_output_key.
Strange because I see no false sharing on this ifindex location for
loopback device.
So we probably can save some cycles adding a net->loopback_ifindex
to remove one dereference.
I am going to let Pavel tackle the actual work because only migration
really cares and he is working on migration right now.
But assuming we merge the per network namespace ifindex counter we
can change net->loopback_dev->ifindex to LOOPBACK_IFINDEX and
define "#define LOOPBACK_IFINDEX 1"
Certainly that works in the initial network namespace today and might be
worth testing.
If ifindex are per network space, I guess we'll need to change
arp_hashfn() or else we'll use some slots more than others.
Darn. I hate being right about there being a few places to fix
up.
ndisc_hashfn also has the same limitation.
Eric
From: David Miller <davem@davemloft.net> Date: 2012-08-02 23:37:37
From: ebiederm@xmission.com (Eric W. Biederman)
Date: Thu, 02 Aug 2012 04:09:39 -0700
Eric Dumazet [off-list ref] writes:
quoted
If ifindex are per network space, I guess we'll need to change
arp_hashfn() or else we'll use some slots more than others.
Darn. I hate being right about there being a few places to fix
up.
ndisc_hashfn also has the same limitation.
And netlabel's inteface hashing as well.
LLC works with ifindex hashing and is not namespace aware. It's
should therefore limited to &init_net and therefore OK. Likewise
for the CAN code.
From: Eric Dumazet <hidden> Date: 2012-08-03 05:45:33
On Thu, 2012-08-02 at 16:26 -0700, David Miller wrote:
From: Eric Dumazet <redacted>
Date: Thu, 02 Aug 2012 12:28:30 +0200
quoted
Strange because I see no false sharing on this ifindex location for
loopback device.
Are you sure netdev->rx_dropped isn't being incremented? That appears
as if it would land on the same cache line as netdev->ifindex.
Yes I am sure (by the way my output device was dummy0, not lo, but its
the same for the case here)
offsetof(struct net_device, features)=0x80
... (all features are mostly read only)
offsetof(struct net_device, ifindex)=0xa0
struct net_device_stats stats; is untouched on dummy device
offsetof(struct net_device, rx_dropped)=0x160
So thats only the dereference done million times per second that show in
the profiles, even if cache lines are clean and in cpu cache.
I see that even more clear in the IN_DEV_ROUTE_LOCALNET(in_dev) macro in
ip_route_input_slow(), doing so many derefs :
Its actually faster to avoid it, if we already have the net pointer in
hand : IN_DEV_NET_ROUTE_LOCALNET(in_dev, net)