Re: [PATCH net-next v3] macvlan: Support for high multicast packet rate
From: Jakub Kicinski <kuba@kernel.org>
Date: 2020-12-01 19:12:30
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 <redacted>
Looks good! Minor nits below:
quoted hunk ↗ jump to hunk
@@ -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?
quoted hunk ↗ jump to hunk
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;
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
}; 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)
{