From mboxrd@z Thu Jan 1 00:00:00 1970 From: ebiederm@xmission.com (Eric W. Biederman) Subject: Re: [patch iproute2 1/6] iproute2: ipa: show switch id Date: Thu, 04 Dec 2014 11:52:49 -0600 Message-ID: <87388v9ua6.fsf@x220.int.ebiederm.org> References: <1417683438-10935-1-git-send-email-jiri@resnulli.us> <1417683438-10935-2-git-send-email-jiri@resnulli.us> <87y4qne6if.fsf@x220.int.ebiederm.org> <20141204163024.GG1861@nanopsycho.orion> Mime-Version: 1.0 Content-Type: text/plain Cc: netdev@vger.kernel.org, davem@davemloft.net, nhorman@tuxdriver.com, andy@greyhouse.net, tgraf@suug.ch, dborkman@redhat.com, ogerlitz@mellanox.com, jesse@nicira.com, pshelar@nicira.com, azhou@nicira.com, ben@decadent.org.uk, stephen@networkplumber.org, jeffrey.t.kirsher@intel.com, vyasevic@redhat.com, xiyou.wangcong@gmail.com, john.r.fastabend@intel.com, edumazet@google.com, jhs@mojatatu.com, sfeldma@gmail.com, f.fainelli@gmail.com, roopa@cumulusnetworks.com, linville@tuxdriver.com, jasowang@redhat.com, nicolas.dichtel@6wind.com, ryazanov.s.a@gmail.com, buytenh@wantstofly.org, aviadr@mellanox.com, nbd@openwrt.org, alexei.starovoitov@gmail.com, Neil.Jerram@metaswitch.com, ronye@mellanox.com, simon.horman@netronome.com, alexander.h.duyck@redhat.com, john.ronciak@intel.com, mleitner@redhat.com, shrijeet@gmail.com, gospo@cumulusnetworks.com, bcrl@kvack.org, hemal@broadcom.c To: Jiri Pirko Return-path: Received: from out03.mta.xmission.com ([166.70.13.233]:46740 "EHLO out03.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932875AbaLDRzM (ORCPT ); Thu, 4 Dec 2014 12:55:12 -0500 In-Reply-To: <20141204163024.GG1861@nanopsycho.orion> (Jiri Pirko's message of "Thu, 4 Dec 2014 17:30:24 +0100") Sender: netdev-owner@vger.kernel.org List-ID: Jiri Pirko writes: > Thu, Dec 04, 2014 at 05:15:04PM CET, ebiederm@xmission.com wrote: >>Jiri Pirko 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 >>> --- >>> 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]));