| Message ID | 4AF19E69.8070605@mellanox.co.il |
|---|---|
| State | Awaiting Upstream, archived |
| Delegated to: | David Miller |
| Headers | show |
> +#define MLX4_MAC_HEAD 0x2c9000000ULL > + random_ether_addr(rand_mac); > + caps->def_mac[i] = MLX4_MAC_HEAD | rand_mac[0] | > + ((u64)(rand_mac[1]) << 8) | ((u64)(rand_mac[2]) << 16); Is this a good idea? You're basically choosing 24 random bits within your OUI... seems the chance of collision with another MAC used on the same network is high enough that it could easily happen in practice on a moderately big network. Can you pick a reserved range or something? Also I'm not sure if there's much point in using random_ether_addr if you're going to override the high order part anyway, since random_ether_addr just makes sure it doesn't pick a multicast address. - R. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Nov 4, 2009 at 10:04 PM, Roland Dreier <rdreier@cisco.com> wrote: >> +#define MLX4_MAC_HEAD 0x2c9000000ULL > Is this a good idea? You're basically choosing 24 random bits within your OUI... > seems the chance of collision with another MAC used on the same network is > high enough that it could easily happen in practice on a moderately big network. yes, this has been brought by Stephen and others on this last back on September 11th, this year @ http://marc.info/?l=linux-netdev&m=125263488409128 > Can you pick a reserved range or something? Using different OUI for the VF device wouldn't help either I think, since the #VF becomes fairly big even on a modest side cluster with (say) a VM consuming VF per 1-2 cores. Or. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
This approach seems to be common practice now (e.g., drivers/net/igb/igb_main.c:1332). In any case, the user can change the randomized mac. --Liran -----Original Message----- From: Or Gerlitz [mailto:or.gerlitz@gmail.com] Sent: Wednesday, November 04, 2009 11:33 PM To: Roland Dreier Cc: Yevgeny Petrilin; linux-rdma@vger.kernel.org; netdev@vger.kernel.org; Liran Liss; Tziporet Koren Subject: Re: [PATCH 19/25] mlx4: Randomizing mac addresses for slaves On Wed, Nov 4, 2009 at 10:04 PM, Roland Dreier <rdreier@cisco.com> wrote: >> +#define MLX4_MAC_HEAD 0x2c9000000ULL > Is this a good idea? You're basically choosing 24 random bits within your OUI... > seems the chance of collision with another MAC used on the same > network is high enough that it could easily happen in practice on a moderately big network. yes, this has been brought by Stephen and others on this last back on September 11th, this year @ http://marc.info/?l=linux-netdev&m=125263488409128 > Can you pick a reserved range or something? Using different OUI for the VF device wouldn't help either I think, since the #VF becomes fairly big even on a modest side cluster with (say) a VM consuming VF per 1-2 cores. Or. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> This approach seems to be common practice now (e.g., drivers/net/igb/igb_main.c:1332). > In any case, the user can change the randomized mac. igb uses the full output of random_ether_addr(). I'd be fine with that. However setting the OUI means you only get 24 bits of randomness which makes a collision a lot more likely. - R. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 05, 2009 at 07:58:12AM -0800, Roland Dreier wrote: > > > This approach seems to be common practice now (e.g., drivers/net/igb/igb_main.c:1332). > > In any case, the user can change the randomized mac. > > igb uses the full output of random_ether_addr(). I'd be fine with > that. However setting the OUI means you only get 24 bits of randomness > which makes a collision a lot more likely. IIRC that was precisely why the OUI isn't used for the igb driver. Perhaps some infrastructure (by which I mean a random_mac() function) is warranted so at least this discussion can be concentrated around that rather than repeating it for each driver that needs random mac addresses. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > igb uses the full output of random_ether_addr(). I'd be fine with > > that. However setting the OUI means you only get 24 bits of randomness > > which makes a collision a lot more likely. > > IIRC that was precisely why the OUI isn't used for the igb driver. > > Perhaps some infrastructure (by which I mean a random_mac() function) > is warranted so at least this discussion can be concentrated around that > rather than repeating it for each driver that needs random mac addresses. What would be the difference between random_mac() and the existing random_ether_addr() function? If one chooses a random address with a given OUI, then with only 24 bits of randomness, the birthday paradox says it takes only a few thousand addresses to get a collision (easy to hit given even a modest-sized virtualization setup). With the 46 bits that random_ether_addr() gives it takes millions of addresses to be likely to get a collision, which is probably comfortable for most ethernets. So it seems that random_ether_addr() is exactly what we should be using for VFs -- the only alternative I see is for the manufacturer to allocate N extra ethernet addresses for a NIC that supports N virtual functions, and use those assigned addresses. But if the kernel is making up ethernet addresses then we better use all the bits we can. - R. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 05, 2009 at 10:08:58PM -0800, Roland Dreier wrote: > > > > igb uses the full output of random_ether_addr(). I'd be fine with > > > that. However setting the OUI means you only get 24 bits of randomness > > > which makes a collision a lot more likely. > > > > IIRC that was precisely why the OUI isn't used for the igb driver. > > > > Perhaps some infrastructure (by which I mean a random_mac() function) > > is warranted so at least this discussion can be concentrated around that > > rather than repeating it for each driver that needs random mac addresses. > > What would be the difference between random_mac() and the existing > random_ether_addr() function? Sorry, I was mistaken. igb is using random_ether_addr(), which is what I had in mind. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/mlx4/fw.c b/drivers/net/mlx4/fw.c index 60889d3..9028592 100644 --- a/drivers/net/mlx4/fw.c +++ b/drivers/net/mlx4/fw.c @@ -32,6 +32,7 @@ * SOFTWARE. */ +#include <linux/etherdevice.h> #include <linux/mlx4/cmd.h> #include <linux/cache.h> @@ -148,6 +149,7 @@ int mlx4_QUERY_SLAVE_CAP_wrapper(struct mlx4_dev *dev, int slave, struct mlx4_vh struct mlx4_cmd_mailbox *outbox) { struct mlx4_caps *caps = outbox->buf; + u8 rand_mac[6]; int i; memcpy(caps, &dev->caps, sizeof *caps); @@ -165,6 +167,10 @@ int mlx4_QUERY_SLAVE_CAP_wrapper(struct mlx4_dev *dev, int slave, struct mlx4_vh for (i = 1; i <= dev->caps.num_ports; ++i) { caps->gid_table_len[i] = 1; caps->pkey_table_len[i] = 1; +#define MLX4_MAC_HEAD 0x2c9000000ULL + random_ether_addr(rand_mac); + caps->def_mac[i] = MLX4_MAC_HEAD | rand_mac[0] | + ((u64)(rand_mac[1]) << 8) | ((u64)(rand_mac[2]) << 16); } } else { caps->sqp_demux = dev->num_slaves;
Signed-off-by: Yevgeny Petrilin <yevgenyp@mellanox.co.il> --- drivers/net/mlx4/fw.c | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-)