netdev - Re: [PATCH net-next v3] macvlan: Support for high multicast packet rate

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20201201111143.2a82d744@kicinski-fedora-pc1c0hjn.DHCP.thefacebook.com>
Date:   Tue, 1 Dec 2020 11:11:43 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Thomas Karlsson <thomas.karlsson@...eda.se>
Cc:     "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        <jiri@...nulli.us>, <kaber@...sh.net>, <edumazet@...gle.com>,
        <vyasevic@...hat.com>, <alexander.duyck@...il.com>
Subject: Re: [PATCH net-next v3] macvlan: Support for high multicast packet
 rate

On Mon, 30 Nov 2020 15:00:43 +0100 Thomas Karlsson wrote:
> Background:
> Broadcast and multicast packages are enqueued for later processing.
> This queue was previously hardcoded to 1000.
> 
> This proved insufficient for handling very high packet rates.
> This resulted in packet drops for multicast.
> While at the same time unicast worked fine.
> 
> The change:
> This patch make the queue length adjustable to accommodate
> for environments with very high multicast packet rate.
> But still keeps the default value of 1000 unless specified.
> 
> The queue length is specified as a request per macvlan
> using the IFLA_MACVLAN_BC_QUEUE_LEN parameter.
> 
> The actual used queue length will then be the maximum of
> any macvlan connected to the same port. The actual used
> queue length for the port can be retrieved (read only)
> by the IFLA_MACVLAN_BC_QUEUE_LEN_USED parameter for verification.
> 
> This will be followed up by a patch to iproute2
> in order to adjust the parameter from userspace.
> 
> Signed-off-by: Thomas Karlsson <thomas.karlsson@...eda.se>

Looks good! Minor nits below:

> @@ -1218,6 +1220,7 @@ static int macvlan_port_create(struct net_device *dev)
>  	for (i = 0; i < MACVLAN_HASH_SIZE; i++)
>  		INIT_HLIST_HEAD(&port->vlan_source_hash[i]);
>  
> +	port->bc_queue_len_used = MACVLAN_DEFAULT_BC_QUEUE_LEN;

Should this be inited to 0? Otherwise if the first link asks for lower
queue len than the default it will not get set, right?

>  	skb_queue_head_init(&port->bc_queue);
>  	INIT_WORK(&port->bc_work, macvlan_process_broadcast);
>  
> @@ -1486,6 +1489,12 @@ int macvlan_common_newlink(struct net *src_net, struct net_device *dev,
>  			goto destroy_macvlan_port;
>  	}
>  
> +	vlan->bc_queue_len_requested = MACVLAN_DEFAULT_BC_QUEUE_LEN;
> +	if (data && data[IFLA_MACVLAN_BC_QUEUE_LEN])
> +		vlan->bc_queue_len_requested = nla_get_u32(data[IFLA_MACVLAN_BC_QUEUE_LEN]);
> +	if (vlan->bc_queue_len_requested > port->bc_queue_len_used)
> +		port->bc_queue_len_used = vlan->bc_queue_len_requested;

Or perhaps we should just call update_port_bc_queue_len() here?

>  	err = register_netdevice(dev);
>  	if (err < 0)
>  		goto destroy_macvlan_port;

> @@ -1658,6 +1684,8 @@ static const struct nla_policy macvlan_policy[IFLA_MACVLAN_MAX + 1] = {
>  	[IFLA_MACVLAN_MACADDR] = { .type = NLA_BINARY, .len = MAX_ADDR_LEN },
>  	[IFLA_MACVLAN_MACADDR_DATA] = { .type = NLA_NESTED },
>  	[IFLA_MACVLAN_MACADDR_COUNT] = { .type = NLA_U32 },
> +	[IFLA_MACVLAN_BC_QUEUE_LEN] = { .type = NLA_U32 },
> +	[IFLA_MACVLAN_BC_QUEUE_LEN_USED] = { .type = NLA_U32 },

This is an input policy, so you can set type to NLA_REJECT and you
won't have to check if it's set on input.

>  };
>  
>  int macvlan_link_register(struct rtnl_link_ops *ops)
> @@ -1688,6 +1716,18 @@ static struct rtnl_link_ops macvlan_link_ops = {
>  	.priv_size      = sizeof(struct macvlan_dev),
>  };
>  
> +static void update_port_bc_queue_len(struct macvlan_port *port)
> +{
> +	struct macvlan_dev *vlan;
> +	u32 max_bc_queue_len_requested = 0;

Please reorder so that the vars are longest line to shortest.

> +	list_for_each_entry_rcu(vlan, &port->vlans, list) {

I don't think you need the _rcu() flavor here, this is always called
from the configuration paths holding RTNL lock, right?

> +		if (vlan->bc_queue_len_requested > max_bc_queue_len_requested)
> +			max_bc_queue_len_requested = vlan->bc_queue_len_requested;
> +	}
> +	port->bc_queue_len_used = max_bc_queue_len_requested;
> +}
> +
>  static int macvlan_device_event(struct notifier_block *unused,
>  				unsigned long event, void *ptr)
>  {