Date:	Thu, 04 Dec 2014 09:59:02 -0800
From:	Roopa Prabhu <roopa@...ulusnetworks.com>
To:	"Eric W. Biederman" <ebiederm@...ssion.com>
CC:	Jiri Pirko <jiri@...nulli.us>, 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, 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

On 12/4/14, 9:52 AM, Eric W. Biederman 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?
>
> 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.

Agree..100%. This was my original concern as well and i have raised it 
earlier.
  Don't expose it to userspace if the semantics are still not clear.

Thanks.

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