Date:	Thu, 4 Dec 2014 19:24:51 +0100
From:	Jiri Pirko <jiri@...nulli.us>
To:	"Eric W. Biederman" <ebiederm@...ssion.com>
Cc:	netdev@...r.kernel.org, davem@...emloft.net, nhorman@...driver.com,
	andy@...yhouse.net, tgraf@...g.ch, dborkman@...hat.com,
	ogerlitz@...lanox.com, jesse@...ira.com, pshelar@...ira.com,
	azhou@...ira.com, ben@...adent.org.uk, stephen@...workplumber.org,
	jeffrey.t.kirsher@...el.com, vyasevic@...hat.com,
	xiyou.wangcong@...il.com, john.r.fastabend@...el.com,
	edumazet@...gle.com, jhs@...atatu.com, sfeldma@...il.com,
	f.fainelli@...il.com, roopa@...ulusnetworks.com,
	linville@...driver.com, jasowang@...hat.com,
	nicolas.dichtel@...nd.com, ryazanov.s.a@...il.com,
	buytenh@...tstofly.org, aviadr@...lanox.com, nbd@...nwrt.org,
	alexei.starovoitov@...il.com, Neil.Jerram@...aswitch.com,
	ronye@...lanox.com, simon.horman@...ronome.com,
	alexander.h.duyck@...hat.com, john.ronciak@...el.com,
	mleitner@...hat.com, shrijeet@...il.com, gospo@...ulusnetworks.com,
	bcrl@...ck.org, hemal@...adcom.com
Subject: Re: [patch iproute2 1/6] iproute2: ipa: show switch id

Thu, Dec 04, 2014 at 06:52:49PM CET, ebiederm@...ssion.com wrote:
>Jiri Pirko <jiri@...nulli.us> writes:
>
>> Thu, Dec 04, 2014 at 05:15:04PM CET, ebiederm@...ssion.com wrote:
>>>Jiri Pirko <jiri@...nulli.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?

It is up to the driver what to use. It can use mac addr. This is same as
for phys port id.


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

Yes it is (as for phys port id).

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

Yes, we are.

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

I don't understand why would net/bridge or driver/net/macvlan want to
implement this. The purpose is to group ports/netdevs which are part of
the same hw switch.

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

Userspace threats this the same it treats phys port id.

if two ports/netdevs has same switch id, they belong under same hw
switch. That's all.


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

I thought that the semantics is clean. Looks like I will have to update
Documentation/networking/switchdev.txt adding some more info about this.

>
>Thanks,
>Eric
>
>
>
>>>> Signed-off-by: Jiri Pirko <jiri@...nulli.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]));
--
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