Re: [patch iproute2 1/6] iproute2: ipa: show switch id

[ebiederm@xmission.com (Eric W. Biederman)]

Jiri Pirko <jiri@resnulli.us> writes:

> Thu, Dec 04, 2014 at 05:15:04PM CET, ebiederm@xmission.com wrote:
>>Jiri Pirko <jiri@resnulli.us> writes:
>>
>>Would someone please explain to me what a switch id is?
>>
>>I looked in the kernel source, and I looked here and while I know
>>switches I don't have a clue what a switch id is.
>>
>>My primary concern at this point is that you have introduced a global
>>identifier that is isn't a hardware property (it certainly does not look
>>like a mac address) and that is unique across network namespaces and
>>thus breaks checkpoint/restart (aka CRIU).
>
> IFLA_PHYS_SWITCH_ID is very similar to IFLA_PHYS_PORT_ID. It is
> generated by the driver and ensures that there is the same switch id for
> all ports belonging to the same switch chip/asic. It is up to the driver
> how to implement the id. I would like to point you to driver
> implementing ndo_get_phys_port_id

Looking at ndo_get_phys_port_id it is just the per port mac address.  Or
guid in the case of infiniband.  Which really makes me wonder why we
didn't use the same abstractions in the code for address types that we
do for hardware addresses.

Using mac address or other hardware addresses that are used for layer 2
addressing makes sense to me.  There is a long tradition of that and as
I recall protocols like STP actually requiring having a different mac
address per port.

When I asked the question I thought the switch id was going to be
something like the ifindex, the software index of a network device.


Finally having tracked down the rocker implementation of 
rocker_port_switch_parent_id_get I see it you are reading some 64bit
hardware register.

Which leads me to ask what are the semantics of switch_id?

Is the switch id an identifier with a prefix from IEEE and assigned by
the manufacture so that it is guaranteed to the tolerances of the
manufacturing process to be globally unique?

Is the switch id a random number that is statistically likely to be
globally unique because you have enough bits?   As I recall you need
at least 128 bits to have a reasonable chance of a random number
avoiding the birthday paradox.

Do we need some kind of manufacturer id to tell one switch id from
another?

Is the switch id persistent across reboots?

>>Also what in the world does PHYS mean in IFLA_PHYS_SWITCH_ID?  Does that
>>mean we can't have a purely software implementation of this interface?
>>Given that we will want a software implementation at some point
>>including PHYS in the name seems completely wrong.
>
> We can remove the "PHYS", no problem. I do not understand what you say
> about "software implementation". The point is to allow hw switch/ish
> chips to be supported.

If we are talking about something typically stored in a eeprom like a
mac address phys seems appropriate.

Still having a definition of this switch id clean clear enough that
net/bridge and drivers/net/macvlan can implement it seems important.

Even more important is having a definition of switch id clear enough
that userspace can use the switch id to do something useful.

Right now switch id looks like one of those weird one manufacturer
properties that is fine to expose as a driver specific property
but I don't yet see it being a generic property I that can be used
usefully in userspace.

So can we please get some clear semantics or failing that can we please
not expose this to userspace as generic property.

Thanks,
Eric



>>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>>> ---
>>>  include/linux/if_link.h | 1 +
>>>  ip/ipaddress.c          | 8 ++++++++
>>>  2 files changed, 9 insertions(+)
>>>
>>> diff --git a/include/linux/if_link.h b/include/linux/if_link.h
>>> index 4732063..a6e2594 100644
>>> --- a/include/linux/if_link.h
>>> +++ b/include/linux/if_link.h
>>> @@ -145,6 +145,7 @@ enum {
>>>  	IFLA_CARRIER,
>>>  	IFLA_PHYS_PORT_ID,
>>>  	IFLA_CARRIER_CHANGES,
>>> +	IFLA_PHYS_SWITCH_ID,
>>>  	__IFLA_MAX
>>>  };
>>>  
>>> diff --git a/ip/ipaddress.c b/ip/ipaddress.c
>>> index 4d99324..bd36a07 100644
>>> --- a/ip/ipaddress.c
>>> +++ b/ip/ipaddress.c
>>> @@ -589,6 +589,14 @@ int print_linkinfo(const struct sockaddr_nl *who,
>>>  				      b1, sizeof(b1)));
>>>  	}
>>>  
>>> +	if (tb[IFLA_PHYS_SWITCH_ID]) {
>>> +		SPRINT_BUF(b1);
>>> +		fprintf(fp, "switchid %s ",
>>> +			hexstring_n2a(RTA_DATA(tb[IFLA_PHYS_SWITCH_ID]),
>>> +				      RTA_PAYLOAD(tb[IFLA_PHYS_SWITCH_ID]),
>>> +				      b1, sizeof(b1)));
>>> +	}
>>> +
>>>  	if (tb[IFLA_OPERSTATE])
>>>  		print_operstate(fp, rta_getattr_u8(tb[IFLA_OPERSTATE]));