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.karls...@paneda.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)
> {
