nft netdev family bindings

nft netdev family bindings

[Patrick McHardy]

Hi Pablo,

looking at the netdev syntax:

table netdev eth0 {
	device eth0;
	...

I think this "device" specification is inconsistent with out normal use
of handles. Usually the table_spec contains the fully qualified handle,
which in this case needs to include the device.

Consider:

table netdev somename {
	device eth0;
	...

table netdev somename {
	device eth1;
	...

Without including the device in the table handle, the name alone is amiguitios.
I'd propose to use

table netdev <dev> <name>

Just as we have the family in the handle.

Re: nft netdev family bindings

[Pablo Neira Ayuso]

On Fri, Jun 05, 2015 at 03:35:33PM +0200, Patrick McHardy wrote:
> Hi Pablo,
> 
> looking at the netdev syntax:
> 
> table netdev eth0 {
> 	device eth0;
> 	...
> 
> I think this "device" specification is inconsistent with out normal use
> of handles. Usually the table_spec contains the fully qualified handle,
> which in this case needs to include the device.
> 
> Consider:
> 
> table netdev somename {
> 	device eth0;
> 	...
> 
> table netdev somename {
> 	device eth1;
> 	...

I see, you mean the same name:

# nft add table netdev somename { device eth0 \; }
# nft add table netdev somename { device eth1 \; }

I can see this is not working fine now, since the second invocation is
considered an update. But the kernel should bail out with EBUSY IMO.

> Without including the device in the table handle, the name alone is amiguitios.

The table name should be unique as with other families. Then, probably
the device doesn't belong to the handle.

> I'd propose to use
> 
> table netdev <dev> <name>
> 
> Just as we have the family in the handle.

I've considering to allow to bind a table to an input device from
other families as something optional. From the hardware offload
perspective we would need this too if we want to offload the
forwarding table.

Let me know, thanks!

Re: nft netdev family bindings

[Patrick McHardy]

On 05.06, Pablo Neira Ayuso wrote:
> On Fri, Jun 05, 2015 at 03:35:33PM +0200, Patrick McHardy wrote:
> > Hi Pablo,
> > 
> > looking at the netdev syntax:
> > 
> > table netdev eth0 {
> > 	device eth0;
> > 	...
> > 
> > I think this "device" specification is inconsistent with out normal use
> > of handles. Usually the table_spec contains the fully qualified handle,
> > which in this case needs to include the device.
> > 
> > Consider:
> > 
> > table netdev somename {
> > 	device eth0;
> > 	...
> > 
> > table netdev somename {
> > 	device eth1;
> > 	...
> 
> I see, you mean the same name:
> 
> # nft add table netdev somename { device eth0 \; }
> # nft add table netdev somename { device eth1 \; }
> 
> I can see this is not working fine now, since the second invocation is
> considered an update. But the kernel should bail out with EBUSY IMO.
> 
> > Without including the device in the table handle, the name alone is amiguitios.
> 
> The table name should be unique as with other families. Then, probably
> the device doesn't belong to the handle.

Yes, I considered every device a single namespace, but thinking about it
again, it seems more consistent to treat netdev as any other family and
treat devices similar to base chains. In that case though, it would be
more consistent to have the device specification not on a table level,
but on a chain level. So we could have multiple devices as base chains
in a single table, just as we have multiple hooks in other families.

> > I'd propose to use
> > 
> > table netdev <dev> <name>
> > 
> > Just as we have the family in the handle.
> 
> I've considering to allow to bind a table to an input device from
> other families as something optional. From the hardware offload
> perspective we would need this too if we want to offload the
> forwarding table.
> 
> Let me know, thanks!

I think we need to hash out how specifically this will work before
we make such decisions. I think the ideas are still to abstract to
add something like this at this point.

Re: nft netdev family bindings

[Patrick McHardy]

On 05.06, Patrick McHardy wrote:
> On 05.06, Pablo Neira Ayuso wrote:
> > > I think this "device" specification is inconsistent with out normal use
> > > of handles. Usually the table_spec contains the fully qualified handle,
> > > which in this case needs to include the device.
> > > 
> > > Consider:
> > > 
> > > table netdev somename {
> > > 	device eth0;
> > > 	...
> > > 
> > > table netdev somename {
> > > 	device eth1;
> > > 	...
> > 
> > I see, you mean the same name:
> > 
> > # nft add table netdev somename { device eth0 \; }
> > # nft add table netdev somename { device eth1 \; }
> > 
> > I can see this is not working fine now, since the second invocation is
> > considered an update. But the kernel should bail out with EBUSY IMO.
> > 
> > > Without including the device in the table handle, the name alone is amiguitios.
> > 
> > The table name should be unique as with other families. Then, probably
> > the device doesn't belong to the handle.
> 
> Yes, I considered every device a single namespace, but thinking about it
> again, it seems more consistent to treat netdev as any other family and
> treat devices similar to base chains. In that case though, it would be
> more consistent to have the device specification not on a table level,
> but on a chain level. So we could have multiple devices as base chains
> in a single table, just as we have multiple hooks in other families.

Ok let's reconsider. For ingress, it definitely seems useful to have
tables which can bind to multiple devices, f.i. for shared sets.

OTOH when using the netdev family for offloading in the future, it
will most likely be impossible to really share data or chains except
when re-expanding it for every device. So the per table binding to
a device makes sense for that.

The point that keeps confusing me is whether we should consider the
device part of the handle, which would implies a seperate namespace
per device, or part of the hook, which would imply a per chain
property, or something completely new, such as a binding. In the
kernel, we have seperate hooks for every device, and for offloading
every device will also have its own namespace, probably even supporting
different features, so I'm tending back to the idea that it should be
part of the handle and every device should be treated as a seperat
namespace, as we currently do for every family.

Re: nft netdev family bindings

[Pablo Neira Ayuso]

Hi Patrick,

On Fri, Jun 05, 2015 at 06:47:45PM +0200, Patrick McHardy wrote:
> On 05.06, Patrick McHardy wrote:
> > On 05.06, Pablo Neira Ayuso wrote:
> > > > I think this "device" specification is inconsistent with out normal use
> > > > of handles. Usually the table_spec contains the fully qualified handle,
> > > > which in this case needs to include the device.
> > > > 
> > > > Consider:
> > > > 
> > > > table netdev somename {
> > > > 	device eth0;
> > > > 	...
> > > > 
> > > > table netdev somename {
> > > > 	device eth1;
> > > > 	...
> > > 
> > > I see, you mean the same name:
> > > 
> > > # nft add table netdev somename { device eth0 \; }
> > > # nft add table netdev somename { device eth1 \; }
> > > 
> > > I can see this is not working fine now, since the second invocation is
> > > considered an update. But the kernel should bail out with EBUSY IMO.
> > > 
> > > > Without including the device in the table handle, the name alone is amiguitios.
> > > 
> > > The table name should be unique as with other families. Then, probably
> > > the device doesn't belong to the handle.
> > 
> > Yes, I considered every device a single namespace, but thinking about it
> > again, it seems more consistent to treat netdev as any other family and
> > treat devices similar to base chains. In that case though, it would be
> > more consistent to have the device specification not on a table level,
> > but on a chain level. So we could have multiple devices as base chains
> > in a single table, just as we have multiple hooks in other families.
> 
> Ok let's reconsider. For ingress, it definitely seems useful to have
> tables which can bind to multiple devices, f.i. for shared sets.

IIRC people are also using ifb as a way for a common policing and
shaping on aggregated links. So this idea of allowing to bind a table
to multiples devices make sense to me.

Then, the idea would to iterate over the list of netdevs that the user
indicates, eg.

table netdev ingress {
        device { eth0, eth1\; }

        ...
}

and register the same chain hooks for each device in the list.

I can go after this and cook a patch for this. The merge window is
still open so we can modify the semantics of the existing netlink
NFTA_TABLE_DEV attribute in David's net-next tree.

> OTOH when using the netdev family for offloading in the future, it
> will most likely be impossible to really share data or chains except
> when re-expanding it for every device. So the per table binding to
> a device makes sense for that.

Yes, the example above will be more complicated to support as there
will be no shared data, and we'll have to repeat the same operation on
several devices. Even a bit more complicated if they are different
device since as soon as one doesn't support one thing, we'll have to
either bail out or fall back to software (AFAIK depending on what ).

Anyway, I don't think we should constrain software because of what we
expect to find in hardware. It should be the other way around,
hardware will have to adapt to the interface that we provide. We'll
have to extend those interfaces incrementally to support more features
if that becomes limited I would say.

> The point that keeps confusing me is whether we should consider the
> device part of the handle, which would implies a seperate namespace
> per device, or part of the hook, which would imply a per chain
> property, or something completely new, such as a binding. In the
> kernel, we have seperate hooks for every device, and for offloading
> every device will also have its own namespace, probably even supporting
> different features, so I'm tending back to the idea that it should be
> part of the handle and every device should be treated as a seperate
> namespace, as we currently do for every family.

So the handle contains the tuple that uniquely identifies the table.
I'm thinking the device doesn't belong there if we support the
scenario above. I see the device statement as a way to bind the table
to a set of devices.

Thanks.

Re: nft netdev family bindings

[Patrick McHardy]

On 08.06, Pablo Neira Ayuso wrote:
> Hi Patrick,
> 
> On Fri, Jun 05, 2015 at 06:47:45PM +0200, Patrick McHardy wrote:
> > On 05.06, Patrick McHardy wrote:
> > > On 05.06, Pablo Neira Ayuso wrote:
> > > > > I think this "device" specification is inconsistent with out normal use
> > > > > of handles. Usually the table_spec contains the fully qualified handle,
> > > > > which in this case needs to include the device.
> > > > > 
> > > > > Consider:
> > > > > 
> > > > > table netdev somename {
> > > > > 	device eth0;
> > > > > 	...
> > > > > 
> > > > > table netdev somename {
> > > > > 	device eth1;
> > > > > 	...
> > > > 
> > > > I see, you mean the same name:
> > > > 
> > > > # nft add table netdev somename { device eth0 \; }
> > > > # nft add table netdev somename { device eth1 \; }
> > > > 
> > > > I can see this is not working fine now, since the second invocation is
> > > > considered an update. But the kernel should bail out with EBUSY IMO.
> > > > 
> > > > > Without including the device in the table handle, the name alone is amiguitios.
> > > > 
> > > > The table name should be unique as with other families. Then, probably
> > > > the device doesn't belong to the handle.
> > > 
> > > Yes, I considered every device a single namespace, but thinking about it
> > > again, it seems more consistent to treat netdev as any other family and
> > > treat devices similar to base chains. In that case though, it would be
> > > more consistent to have the device specification not on a table level,
> > > but on a chain level. So we could have multiple devices as base chains
> > > in a single table, just as we have multiple hooks in other families.
> > 
> > Ok let's reconsider. For ingress, it definitely seems useful to have
> > tables which can bind to multiple devices, f.i. for shared sets.
> 
> IIRC people are also using ifb as a way for a common policing and
> shaping on aggregated links. So this idea of allowing to bind a table
> to multiples devices make sense to me.
> 
> Then, the idea would to iterate over the list of netdevs that the user
> indicates, eg.
> 
> table netdev ingress {
>         device { eth0, eth1\; }
> 
>         ...
> }
> 
> and register the same chain hooks for each device in the list.
> 
> I can go after this and cook a patch for this. The merge window is
> still open so we can modify the semantics of the existing netlink
> NFTA_TABLE_DEV attribute in David's net-next tree.

My idea was to have the base chains bind to a device, then we can
create shared chains and jump to them from the base chain:

table netdev ingress {
	chain eth0 {
		hook eth0 ingress;
		jump shared_chain;
	}
	chain eth1 {
		hook eth1 ingress;
		jump shared_chain;
	}
	chain shared_chain {
		...
	}
}

I think if we treat the table namespace global, than the hook and base
chain is the natural place to specify the device since this is where
the packets actually enter.

> > OTOH when using the netdev family for offloading in the future, it
> > will most likely be impossible to really share data or chains except
> > when re-expanding it for every device. So the per table binding to
> > a device makes sense for that.
> 
> Yes, the example above will be more complicated to support as there
> will be no shared data, and we'll have to repeat the same operation on
> several devices. Even a bit more complicated if they are different
> device since as soon as one doesn't support one thing, we'll have to
> either bail out or fall back to software (AFAIK depending on what ).
> 
> Anyway, I don't think we should constrain software because of what we
> expect to find in hardware. It should be the other way around,
> hardware will have to adapt to the interface that we provide. We'll
> have to extend those interfaces incrementally to support more features
> if that becomes limited I would say.

Well, hardware can not really adapt to us, but we can of course refuse
operations if they are not supported by hardware if offloading was
explicitly requested.

> > The point that keeps confusing me is whether we should consider the
> > device part of the handle, which would implies a seperate namespace
> > per device, or part of the hook, which would imply a per chain
> > property, or something completely new, such as a binding. In the
> > kernel, we have seperate hooks for every device, and for offloading
> > every device will also have its own namespace, probably even supporting
> > different features, so I'm tending back to the idea that it should be
> > part of the handle and every device should be treated as a seperate
> > namespace, as we currently do for every family.
> 
> So the handle contains the tuple that uniquely identifies the table.
> I'm thinking the device doesn't belong there if we support the
> scenario above. I see the device statement as a way to bind the table
> to a set of devices.

I don't think we really need that. We can either use per-device namespaces
and put it in the handle, or specify it for the hooking point as part
of the base chain definition.

Re: nft netdev family bindings

[Pablo Neira Ayuso]

On Tue, Jun 09, 2015 at 11:23:19AM +0200, Patrick McHardy wrote:
> On 08.06, Pablo Neira Ayuso wrote:
[...]
> > Then, the idea would to iterate over the list of netdevs that the user
> > indicates, eg.
> > 
> > table netdev ingress {
> >         device { eth0, eth1\; }
> > 
> >         ...
> > }
> > 
> > and register the same chain hooks for each device in the list.
> > 
> > I can go after this and cook a patch for this. The merge window is
> > still open so we can modify the semantics of the existing netlink
> > NFTA_TABLE_DEV attribute in David's net-next tree.
> 
> My idea was to have the base chains bind to a device, then we can
> create shared chains and jump to them from the base chain:
> 
> table netdev ingress {
> 	chain eth0 {
> 		hook eth0 ingress;
> 		jump shared_chain;
> 	}
> 	chain eth1 {
> 		hook eth1 ingress;
> 		jump shared_chain;
> 	}
> 	chain shared_chain {
> 		...
> 	}
> }
> 
> I think if we treat the table namespace global, than the hook and base
> chain is the natural place to specify the device since this is where
> the packets actually enter.

We can also achieve this sharing with the approach I describe above, I
don't see any limitation on that.

If the user doesn't want to share any set and chain, he creates a
separate table so there's a clear separation between namespaces.
Thus, we don't allow user to make convoluted configurations in
scenarios where he doesn't need to share anything.

Re: nft netdev family bindings

[Patrick McHardy]

On 09.06, Pablo Neira Ayuso wrote:
> On Tue, Jun 09, 2015 at 11:23:19AM +0200, Patrick McHardy wrote:
> > My idea was to have the base chains bind to a device, then we can
> > create shared chains and jump to them from the base chain:
> > 
> > table netdev ingress {
> > 	chain eth0 {
> > 		hook eth0 ingress;
> > 		jump shared_chain;
> > 	}
> > 	chain eth1 {
> > 		hook eth1 ingress;
> > 		jump shared_chain;
> > 	}
> > 	chain shared_chain {
> > 		...
> > 	}
> > }
> > 
> > I think if we treat the table namespace global, than the hook and base
> > chain is the natural place to specify the device since this is where
> > the packets actually enter.
> 
> We can also achieve this sharing with the approach I describe above, I
> don't see any limitation on that.

Its not a limitation, merely something that seems slightly inconsistent.
The base chain definitions basically define the entry points and in this
case we define them in two steps, unlike in the other cases.

It also matches better what we have in the kernel. Its actually the
hooks that are per device and nothing else.

> If the user doesn't want to share any set and chain, he creates a
> separate table so there's a clear separation between namespaces.
> Thus, we don't allow user to make convoluted configurations in
> scenarios where he doesn't need to share anything.

Yes, but that wouldn't be different if we have the device binding in
the hook definition.

Re: nft netdev family bindings

[Pablo Neira Ayuso]

On Tue, Jun 09, 2015 at 12:57:12PM +0200, Patrick McHardy wrote:
> On 09.06, Pablo Neira Ayuso wrote:
> > 
> > We can also achieve this sharing with the approach I describe above, I
> > don't see any limitation on that.
> 
> Its not a limitation, merely something that seems slightly inconsistent.
> The base chain definitions basically define the entry points and in this
> case we define them in two steps, unlike in the other cases.
> 
> It also matches better what we have in the kernel. Its actually the
> hooks that are per device and nothing else.

It's an abstraction so it's normal that we get out a bit from the hook
representation in the kernel. Chains are not a 1:1 map with the hook
object anyway.

I think this representation is a bit more compact:

table netdev global {
        device { eth0, eth1; }

        chain ingress {
                type filter hook ingress priority 0\;
                ... your rules here, no need for shared_chain ...
        }
}

than this:

table netdev global {
	chain eth0 {
		hook eth0 ingress;
		jump shared_chain;
	}
	chain eth1 {
		hook eth1 ingress;
		jump shared_chain;
	}
	chain shared_chain {
		... your rules here ...
	}
}

If we aim to also have an egress hooks as well, then this may look a
bit convoluted.

On the performance front, this also saves some extra overhead on the
packet path since we don't need the chain jump.

Re: nft netdev family bindings

[Patrick McHardy]

On 09.06, Pablo Neira Ayuso wrote:
> On Tue, Jun 09, 2015 at 12:57:12PM +0200, Patrick McHardy wrote:
> > On 09.06, Pablo Neira Ayuso wrote:
> > > 
> > > We can also achieve this sharing with the approach I describe above, I
> > > don't see any limitation on that.
> > 
> > Its not a limitation, merely something that seems slightly inconsistent.
> > The base chain definitions basically define the entry points and in this
> > case we define them in two steps, unlike in the other cases.
> > 
> > It also matches better what we have in the kernel. Its actually the
> > hooks that are per device and nothing else.
> 
> It's an abstraction so it's normal that we get out a bit from the hook
> representation in the kernel. Chains are not a 1:1 map with the hook
> object anyway.

The question is if its the correct abstraction, and I don't think so.
The device binding doesn't apply to sets, it doesn't apply to chains,
it doesn't apply to any table objects except for hooks, and this is
exactly where we already have the concept of a binding. I mean, the
binding is not even a binding. Nothing is bound to the device, it
is in fact a hook specification.

We even have the concept of multiple bindings by using multiple base
chains, which would have to be duplicated for your multiple device
binding idea.

> I think this representation is a bit more compact:
> 
> table netdev global {
>         device { eth0, eth1; }
> 
>         chain ingress {
>                 type filter hook ingress priority 0\;
>                 ... your rules here, no need for shared_chain ...
>         }
> }
> 
> than this:
> 
> table netdev global {
> 	chain eth0 {
> 		hook eth0 ingress;
> 		jump shared_chain;
> 	}
> 	chain eth1 {
> 		hook eth1 ingress;
> 		jump shared_chain;
> 	}
> 	chain shared_chain {
> 		... your rules here ...
> 	}
> }
> 
> If we aim to also have an egress hooks as well, then this may look a
> bit convoluted.
> 
> On the performance front, this also saves some extra overhead on the
> packet path since we don't need the chain jump.

It really depends. Assuming (which is not unlikely) that people will also
have device specific rules they will create extra chains per device
and jump to them. In the end, it really depends on the use case.

Re: nft netdev family bindings

[Pablo Neira Ayuso]

On Tue, Jun 09, 2015 at 02:13:25PM +0200, Patrick McHardy wrote:
[...]
> The question is if its the correct abstraction, and I don't think so.

Your call Patrick, I'll send a patch to move this attribute to the
base chain.

Re: nft netdev family bindings

[Patrick McHardy]

On 10.06, Pablo Neira Ayuso wrote:
> On Tue, Jun 09, 2015 at 02:13:25PM +0200, Patrick McHardy wrote:
> [...]
> > The question is if its the correct abstraction, and I don't think so.
> 
> Your call Patrick, I'll send a patch to move this attribute to the
> base chain.

In my opinion it would make sense to use one of the NFTA_HOOK_ attributes
for this. Unless you can think of a reason where we would want to have
global tables but per device data. I personally can't.