Re: df2b419a4105 - main - ifnet: add if_foreach_sleep() to allow ifnet iterations with sleep.

In reply to: Alexander Chernikov : "Re: df2b419a4105 - main - ifnet: add if_foreach_sleep() to allow ifnet iterations with sleep."
Go to: [ bottom of page ] [ top of archives ] [ this month ]
From: Ravi Pokala <rpokala_at_freebsd.org>
Date: Mon, 06 Mar 2023 19:03:38 UTC
> Naming is hard :-) IMO if_foreach_sleepable marks the intent better . Mind adding you suggestion to the original review? If there are no opposing concerns after a couple of days, I’ll rename it.

Done. Thank you! :-)

-Ravi (rpokala@)

-----Original Message-----
From: Alexander Chernikov <melifaro@FreeBSD.org <mailto:melifaro@FreeBSD.org>>
Date: 2023-03-06, Monday at 09:06
To: Ravi Pokala <rpokala@freebsd.org <mailto:rpokala@freebsd.org>>
Cc: "<src-committers@freebsd.org <mailto:src-committers@freebsd.org>>" <src-committers@FreeBSD.org <mailto:src-committers@FreeBSD.org>>, "<dev-commits-src-all@freebsd.org <mailto:dev-commits-src-all@freebsd.org>>" <dev-commits-src-all@FreeBSD.org <mailto:dev-commits-src-all@FreeBSD.org>>, "<dev-commits-src-main@freebsd.org <mailto:dev-commits-src-main@freebsd.org>>" <dev-commits-src-main@FreeBSD.org <mailto:dev-commits-src-main@FreeBSD.org>>
Subject: Re: df2b419a4105 - main - ifnet: add if_foreach_sleep() to allow ifnet iterations with sleep.


> On 6 Mar 2023, at 16:35, Ravi Pokala <rpokala@freebsd.org <mailto:rpokala@freebsd.org>> wrote:
> 
> Hi Alexander,
Hi Ravi,
> 
> Knowing ~nothing about ifnet, if I were to see 'if_foreach_sleep()', my first thought would be that the loop itself was sleeping between each iteration, not that the callback was allowed to sleep. Perhaps this should be renamed to 'if_foreach_sleepable()’?
Naming is hard :-) IMO if_foreach_sleepable marks the intent better . Mind adding you suggestion to the original review? If there are no opposing concerns after a couple of days, I’ll rename it.
> 
> Just a thought.
> 
> Thanks,
> 
> Ravi (rpokala@)
> 
> -----Original Message-----
> From: <owner-src-committers@freebsd.org <mailto:owner-src-committers@freebsd.org> <mailto:owner-src-committers@freebsd.org <mailto:owner-src-committers@freebsd.org>>> on behalf of "Alexander V. Chernikov" <melifaro@FreeBSD.org <mailto:melifaro@FreeBSD.org> <mailto:melifaro@FreeBSD.org <mailto:melifaro@FreeBSD.org>>>
> Date: 2023-03-06, Monday at 07:08
> To: <src-committers@FreeBSD.org <mailto:src-committers@FreeBSD.org> <mailto:src-committers@FreeBSD.org <mailto:src-committers@FreeBSD.org>>>, <dev-commits-src-all@FreeBSD.org <mailto:dev-commits-src-all@FreeBSD.org> <mailto:dev-commits-src-all@FreeBSD.org <mailto:dev-commits-src-all@FreeBSD.org>>>, <dev-commits-src-main@FreeBSD.org <mailto:dev-commits-src-main@FreeBSD.org> <mailto:dev-commits-src-main@FreeBSD.org <mailto:dev-commits-src-main@FreeBSD.org>>>
> Subject: git: df2b419a4105 - main - ifnet: add if_foreach_sleep() to allow ifnet iterations with sleep.
> 
> 
> The branch main has been updated by melifaro:
> 
> 
> URL: https://cgit.FreeBSD.org/src/commit/?id=df2b419a4105588384a89a57442ed6c6ca002455 <https://cgit.FreeBSD.org/src/commit/?id=df2b419a4105588384a89a57442ed6c6ca002455> <https://cgit.FreeBSD.org/src/commit/?id=df2b419a4105588384a89a57442ed6c6ca002455> <https://cgit.FreeBSD.org/src/commit/?id=df2b419a4105588384a89a57442ed6c6ca002455&gt;>
> 
> 
> commit df2b419a4105588384a89a57442ed6c6ca002455
> Author: Alexander V. Chernikov <melifaro@FreeBSD.org <mailto:melifaro@FreeBSD.org> <mailto:melifaro@FreeBSD.org <mailto:melifaro@FreeBSD.org>>>
> AuthorDate: 2023-03-04 10:09:09 +0000
> Commit: Alexander V. Chernikov <melifaro@FreeBSD.org <mailto:melifaro@FreeBSD.org> <mailto:melifaro@FreeBSD.org <mailto:melifaro@FreeBSD.org>>>
> CommitDate: 2023-03-06 15:08:08 +0000
> 
> 
> ifnet: add if_foreach_sleep() to allow ifnet iterations with sleep.
> 
> 
> Subscribers: imp, ae, glebius
> 
> 
> Differential Revision: https://reviews.freebsd.org/D38904 <https://reviews.freebsd.org/D38904> <https://reviews.freebsd.org/D38904> <https://reviews.freebsd.org/D38904&gt;>
> ---
> sys/net/if.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> sys/net/if_var.h | 2 ++
> 2 files changed, 67 insertions(+)
> 
> 
> diff --git a/sys/net/if.c b/sys/net/if.c
> index 58711061eb5e..f3ef822178ff 100644
> --- a/sys/net/if.c
> +++ b/sys/net/if.c
> @@ -4535,6 +4535,71 @@ if_foreach(if_foreach_cb_t cb, void *cb_arg)
> return (error);
> }
> 
> 
> +/*
> + * Iterates over the list of interfaces, permitting callback function @cb to sleep.
> + * Stops iteration if @cb returns non-zero error code.
> + * Returns the last error code from @cb.
> + * @match_cb: optional match callback limiting the iteration to only matched interfaces
> + * @match_arg: argument to pass to @match_cb
> + * @cb: iteration callback
> + * @cb_arg: argument to pass to @cb
> + */
> +int
> +if_foreach_sleep(if_foreach_match_t match_cb, void *match_arg, if_foreach_cb_t cb,
> + void *cb_arg)
> +{
> + int match_count = 0, array_size = 16; /* 128 bytes for malloc */
> + struct ifnet **match_array = NULL;
> + int error = 0;
> +
> + MPASS(cb);
> +
> + while (true) {
> + struct ifnet **new_array;
> + int new_size = array_size;
> + struct epoch_tracker et;
> + struct ifnet *ifp;
> +
> + while (new_size < match_count)
> + new_size *= 2;
> + new_array = malloc(new_size * sizeof(void *), M_TEMP, M_WAITOK);
> + if (match_array != NULL)
> + memcpy(new_array, match_array, array_size * sizeof(void *));
> + free(match_array, M_TEMP);
> + match_array = new_array;
> + array_size = new_size;
> +
> + match_count = 0;
> + NET_EPOCH_ENTER(et);
> + CK_STAILQ_FOREACH(ifp, &V_ifnet, if_link) {
> + if (match_cb != NULL && !match_cb(ifp, match_arg))
> + continue;
> + if (match_count < array_size) {
> + if (if_try_ref(ifp))
> + match_array[match_count++] = ifp;
> + } else
> + match_count++;
> + }
> + NET_EPOCH_EXIT(et);
> +
> + if (match_count > array_size) {
> + for (int i = 0; i < array_size; i++)
> + if_rele(match_array[i]);
> + continue;
> + } else {
> + for (int i = 0; i < match_count; i++) {
> + if (error == 0)
> + error = cb(match_array[i], cb_arg);
> + if_rele(match_array[i]);
> + }
> + free(match_array, M_TEMP);
> + break;
> + }
> + }
> +
> + return (error);
> +}
> +
> u_int
> if_foreach_lladdr(if_t ifp, iflladdr_cb_t cb, void *cb_arg)
> {
> diff --git a/sys/net/if_var.h b/sys/net/if_var.h
> index c9b2de736d10..3e4d6c883c13 100644
> --- a/sys/net/if_var.h
> +++ b/sys/net/if_var.h
> @@ -680,7 +680,9 @@ typedef u_int if_addr_cb_t(void *, struct ifaddr *, u_int);
> u_int if_foreach_addr_type(if_t ifp, int type, if_addr_cb_t cb, void *cb_arg);
> 
> 
> typedef int (*if_foreach_cb_t)(if_t, void *);
> +typedef bool (*if_foreach_match_t)(if_t, void *);
> int if_foreach(if_foreach_cb_t, void *);
> +int if_foreach_sleep(if_foreach_match_t, void *, if_foreach_cb_t, void *);
> 
> 
> /* Functions */
> void if_setinitfn(if_t ifp, if_init_fn_t);
> 
> 
> 
>