[MPTCP] Re: [MPTCP][PATCH v3 mptcp-next 1/2] mptcp: add the address ID assignment bitmap

[MPTCP] Re: [MPTCP][PATCH v3 mptcp-next 1/2] mptcp: add the address ID assignment bitmap

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 5846 bytes --]

On Tue, 8 Dec 2020, Geliang Tang wrote:

> Currently the address ID set by the netlink PM from user-space is
> overridden by the kernel. This patch added the address ID assignment
> bitmap to allow user-space to set the address ID.
>
> Use a per netns bitmask id_bitmap (256 bits) to keep track of in-use IDs.
> And use next_id to keep track of the highest ID currently in use. If the
> user-space provides an ID at endpoint creation time, try to use it. If
> already in use, endpoint creation fails. Otherwise pick the first ID
> available after the highest currently in use, with wrap-around.
>
> Signed-off-by: Geliang Tang <geliangtang(a)gmail.com>
> ---
> net/mptcp/pm_netlink.c | 65 ++++++++++++++++++++++++++++++------------
> 1 file changed, 47 insertions(+), 18 deletions(-)
>
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index a6d983d80576..e7cafca2d42e 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -36,6 +36,9 @@ struct mptcp_pm_add_entry {
> 	u8			retrans_times;
> };
>
> +#define MAX_ADDR_ID		255
> +#define BITMAP_SZ DIV_ROUND_UP(MAX_ADDR_ID + 1, BITS_PER_LONG)
> +
> struct pm_nl_pernet {
> 	/* protects pernet updates */
> 	spinlock_t		lock;
> @@ -46,6 +49,7 @@ struct pm_nl_pernet {
> 	unsigned int		local_addr_max;
> 	unsigned int		subflows_max;
> 	unsigned int		next_id;
> +	unsigned long		id_bitmap[BITMAP_SZ];
> };
>
> #define MPTCP_PM_ADDR_MAX	8
> @@ -524,10 +528,12 @@ static int mptcp_pm_nl_append_new_local_addr(struct pm_nl_pernet *pernet,
> 	/* to keep the code simple, don't do IDR-like allocation for address ID,
> 	 * just bail when we exceed limits
> 	 */
> -	if (pernet->next_id > 255)
> -		goto out;
> +	if (pernet->next_id == MAX_ADDR_ID)
> +		pernet->next_id = 1;
> 	if (pernet->addrs >= MPTCP_PM_ADDR_MAX)
> 		goto out;
> +	if (test_bit(entry->addr.id, pernet->id_bitmap))
> +		goto out;
>
> 	/* do not insert duplicate address, differentiate on port only
> 	 * singled addresses
> @@ -544,7 +550,19 @@ static int mptcp_pm_nl_append_new_local_addr(struct pm_nl_pernet *pernet,
> 	if (entry->addr.flags & MPTCP_PM_ADDR_FLAG_SUBFLOW)
> 		pernet->local_addr_max++;
>
> -	entry->addr.id = pernet->next_id++;
> +	if (!entry->addr.id) {
> +find_next:
> +		entry->addr.id = find_next_zero_bit(pernet->id_bitmap,
> +						    MAX_ADDR_ID + 1,
> +						    pernet->next_id);
> +		if (entry->addr.id <= 0 || entry->addr.id > MAX_ADDR_ID) {

Hi Geliang -

It looks like pernet->next_id is always 1 or larger, and 
find_next_zero_bit() returns an unsigned value, so it's not necessary to 
check for 'entry->addr.id <= 0'.

Since MAX_ADDR_ID > MPTCP_PM_ADDR_MAX, this shouldn't be an infinite loop. 
Still, there's no reason to check again if pernet->next_id was already 1, 
so I suggest

if (entry_addr.id > MAX_ADDR_ID && pernet->next_id != 1)

to be extra cautious about this loop.


Mat

> +			pernet->next_id = 1;
> +			goto find_next;
> +		}
> +	}
> +	__set_bit(entry->addr.id, pernet->id_bitmap);
> +	if (entry->addr.id > pernet->next_id)
> +		pernet->next_id = entry->addr.id;
> 	pernet->addrs++;
> 	list_add_tail_rcu(&entry->list, &pernet->local_addr_list);
> 	ret = entry->addr.id;
> @@ -857,6 +875,7 @@ static int mptcp_nl_cmd_del_addr(struct sk_buff *skb, struct genl_info *info)
>
> 	pernet->addrs--;
> 	list_del_rcu(&entry->list);
> +	__clear_bit(entry->addr.id, pernet->id_bitmap);
> 	spin_unlock_bh(&pernet->lock);
>
> 	mptcp_nl_remove_subflow_and_signal_addr(sock_net(skb->sk), &entry->addr);
> @@ -894,6 +913,8 @@ static int mptcp_nl_cmd_flush_addrs(struct sk_buff *skb, struct genl_info *info)
> 	spin_lock_bh(&pernet->lock);
> 	list_splice_init(&pernet->local_addr_list, &free_list);
> 	__reset_counters(pernet);
> +	pernet->next_id = 1;
> +	bitmap_zero(pernet->id_bitmap, MAX_ADDR_ID + 1);
> 	spin_unlock_bh(&pernet->lock);
> 	__flush_addrs(sock_net(skb->sk), &free_list);
> 	return 0;
> @@ -994,27 +1015,34 @@ static int mptcp_nl_cmd_dump_addrs(struct sk_buff *msg,
> 	struct pm_nl_pernet *pernet;
> 	int id = cb->args[0];
> 	void *hdr;
> +	int i;
>
> 	pernet = net_generic(net, pm_nl_pernet_id);
>
> 	spin_lock_bh(&pernet->lock);
> -	list_for_each_entry(entry, &pernet->local_addr_list, list) {
> -		if (entry->addr.id <= id)
> -			continue;
> -
> -		hdr = genlmsg_put(msg, NETLINK_CB(cb->skb).portid,
> -				  cb->nlh->nlmsg_seq, &mptcp_genl_family,
> -				  NLM_F_MULTI, MPTCP_PM_CMD_GET_ADDR);
> -		if (!hdr)
> -			break;
> +	for (i = id; i < MAX_ADDR_ID + 1; i++) {
> +		if (test_bit(i, pernet->id_bitmap)) {
> +			entry = __lookup_addr_by_id(pernet, i);
> +			if (!entry)
> +				break;
> +
> +			if (entry->addr.id <= id)
> +				continue;
> +
> +			hdr = genlmsg_put(msg, NETLINK_CB(cb->skb).portid,
> +					  cb->nlh->nlmsg_seq, &mptcp_genl_family,
> +					  NLM_F_MULTI, MPTCP_PM_CMD_GET_ADDR);
> +			if (!hdr)
> +				break;
> +
> +			if (mptcp_nl_fill_addr(msg, entry) < 0) {
> +				genlmsg_cancel(msg, hdr);
> +				break;
> +			}
>
> -		if (mptcp_nl_fill_addr(msg, entry) < 0) {
> -			genlmsg_cancel(msg, hdr);
> -			break;
> +			id = entry->addr.id;
> +			genlmsg_end(msg, hdr);
> 		}
> -
> -		id = entry->addr.id;
> -		genlmsg_end(msg, hdr);
> 	}
> 	spin_unlock_bh(&pernet->lock);
>
> @@ -1148,6 +1176,7 @@ static int __net_init pm_nl_init_net(struct net *net)
> 	INIT_LIST_HEAD_RCU(&pernet->local_addr_list);
> 	__reset_counters(pernet);
> 	pernet->next_id = 1;
> +	bitmap_zero(pernet->id_bitmap, MAX_ADDR_ID + 1);
> 	spin_lock_init(&pernet->lock);
> 	return 0;
> }
> -- 
> 2.26.2
> _______________________________________________
> mptcp mailing list -- mptcp(a)lists.01.org
> To unsubscribe send an email to mptcp-leave(a)lists.01.org
>

--
Mat Martineau
Intel