Re: push kernel lock inside ifioctl_get()
On Tue, Nov 08, 2022 at 08:31:16PM +0300, Vitaliy Makkoveev wrote: > No reason to keep kernel lock around if_clone_list() call. Yes, that is why I will remove it in the very next commit. This way there is one "move existing lock, make things clearer" and one "actually remove a lock for a specific code path" change.
Re: push kernel lock inside ifioctl_get()
No reason to keep kernel lock around if_clone_list() call. > On 8 Nov 2022, at 20:27, Klemens Nanni wrote: > > On Tue, Nov 08, 2022 at 08:04:16PM +0300, Vitaliy Makkoveev wrote: >> The `if_cloners’ list is immutable. You don't need kernel lock >> around if_clone_list() call. >> >>> case SIOCIFGCLONERS: >>> + KERNEL_LOCK(); >>> error = if_clone_list((struct if_clonereq *)data); >>> + KERNEL_UNLOCK(); >>> return (error); >> >> >> With this fix diff looks good for me. > > This is going the be first unlocking diff, I just want to clearly > separate setup churn (like this) from actual lock semantic changes. >
Re: push kernel lock inside ifioctl_get()
On Tue, Nov 08, 2022 at 04:47:23PM +, Martin Pieuchot wrote:
> On 08/11/22(Tue) 15:28, Klemens Nanni wrote:
> > After this mechanical move, I can unlock the individual SIOCG* in there.
>
> I'd suggest grabbing the KERNEL_LOCK() after NET_LOCK_SHARED().
> Otherwise you might spin for the first one then release it when going
> to sleep.
I can do that inside the first switch, but we must grab the net lock
before if_unit() which is called before grabbing the shared net lock.
I can shuffle this around if you really want, or I can simply move the
existing kernel lock further down and keep it in the same order just
like it is now already.
>
> > OK?
> >
> > Index: if.c
> > ===
> > RCS file: /cvs/src/sys/net/if.c,v
> > retrieving revision 1.667
> > diff -u -p -r1.667 if.c
> > --- if.c8 Nov 2022 15:20:24 - 1.667
> > +++ if.c8 Nov 2022 15:26:07 -
> > @@ -2426,33 +2426,43 @@ ifioctl_get(u_long cmd, caddr_t data)
> > size_t bytesdone;
> > const char *label;
> >
> > - KERNEL_LOCK();
> > -
> > switch(cmd) {
> > case SIOCGIFCONF:
> > + KERNEL_LOCK();
> > NET_LOCK_SHARED();
> > error = ifconf(data);
> > NET_UNLOCK_SHARED();
> > + KERNEL_UNLOCK();
> > return (error);
> > case SIOCIFGCLONERS:
> > + KERNEL_LOCK();
> > error = if_clone_list((struct if_clonereq *)data);
> > + KERNEL_UNLOCK();
> > return (error);
> > case SIOCGIFGMEMB:
> > + KERNEL_LOCK();
> > NET_LOCK_SHARED();
> > error = if_getgroupmembers(data);
> > NET_UNLOCK_SHARED();
> > + KERNEL_UNLOCK();
> > return (error);
> > case SIOCGIFGATTR:
> > + KERNEL_LOCK();
> > NET_LOCK_SHARED();
> > error = if_getgroupattribs(data);
> > NET_UNLOCK_SHARED();
> > + KERNEL_UNLOCK();
> > return (error);
> > case SIOCGIFGLIST:
> > + KERNEL_LOCK();
> > NET_LOCK_SHARED();
> > error = if_getgrouplist(data);
> > NET_UNLOCK_SHARED();
> > + KERNEL_UNLOCK();
> > return (error);
> > }
> > +
> > + KERNEL_LOCK();
> >
> > ifp = if_unit(ifr->ifr_name);
> > if (ifp == NULL) {
> >
>
Re: push kernel lock inside ifioctl_get()
On Tue, Nov 08, 2022 at 08:04:16PM +0300, Vitaliy Makkoveev wrote: > The `if_cloners’ list is immutable. You don't need kernel lock > around if_clone_list() call. > > > case SIOCIFGCLONERS: > > + KERNEL_LOCK(); > > error = if_clone_list((struct if_clonereq *)data); > > + KERNEL_UNLOCK(); > > return (error); > > > With this fix diff looks good for me. This is going the be first unlocking diff, I just want to clearly separate setup churn (like this) from actual lock semantic changes.
Re: push kernel lock inside ifioctl_get()
The `if_cloners’ list is immutable. You don't need kernel lock
around if_clone_list() call.
> case SIOCIFGCLONERS:
> + KERNEL_LOCK();
> error = if_clone_list((struct if_clonereq *)data);
> + KERNEL_UNLOCK();
> return (error);
With this fix diff looks good for me.
> On 8 Nov 2022, at 18:28, Klemens Nanni wrote:
>
> After this mechanical move, I can unlock the individual SIOCG* in there.
>
> OK?
>
> Index: if.c
> ===
> RCS file: /cvs/src/sys/net/if.c,v
> retrieving revision 1.667
> diff -u -p -r1.667 if.c
> --- if.c 8 Nov 2022 15:20:24 - 1.667
> +++ if.c 8 Nov 2022 15:26:07 -
> @@ -2426,33 +2426,43 @@ ifioctl_get(u_long cmd, caddr_t data)
> size_t bytesdone;
> const char *label;
>
> - KERNEL_LOCK();
> -
> switch(cmd) {
> case SIOCGIFCONF:
> + KERNEL_LOCK();
> NET_LOCK_SHARED();
> error = ifconf(data);
> NET_UNLOCK_SHARED();
> + KERNEL_UNLOCK();
> return (error);
> case SIOCIFGCLONERS:
> + KERNEL_LOCK();
> error = if_clone_list((struct if_clonereq *)data);
> + KERNEL_UNLOCK();
> return (error);
> case SIOCGIFGMEMB:
> + KERNEL_LOCK();
> NET_LOCK_SHARED();
> error = if_getgroupmembers(data);
> NET_UNLOCK_SHARED();
> + KERNEL_UNLOCK();
> return (error);
> case SIOCGIFGATTR:
> + KERNEL_LOCK();
> NET_LOCK_SHARED();
> error = if_getgroupattribs(data);
> NET_UNLOCK_SHARED();
> + KERNEL_UNLOCK();
> return (error);
> case SIOCGIFGLIST:
> + KERNEL_LOCK();
> NET_LOCK_SHARED();
> error = if_getgrouplist(data);
> NET_UNLOCK_SHARED();
> + KERNEL_UNLOCK();
> return (error);
> }
> +
> + KERNEL_LOCK();
>
> ifp = if_unit(ifr->ifr_name);
> if (ifp == NULL) {
>
Re: push kernel lock inside ifioctl_get()
On 08/11/22(Tue) 15:28, Klemens Nanni wrote:
> After this mechanical move, I can unlock the individual SIOCG* in there.
I'd suggest grabbing the KERNEL_LOCK() after NET_LOCK_SHARED().
Otherwise you might spin for the first one then release it when going
to sleep.
> OK?
>
> Index: if.c
> ===
> RCS file: /cvs/src/sys/net/if.c,v
> retrieving revision 1.667
> diff -u -p -r1.667 if.c
> --- if.c 8 Nov 2022 15:20:24 - 1.667
> +++ if.c 8 Nov 2022 15:26:07 -
> @@ -2426,33 +2426,43 @@ ifioctl_get(u_long cmd, caddr_t data)
> size_t bytesdone;
> const char *label;
>
> - KERNEL_LOCK();
> -
> switch(cmd) {
> case SIOCGIFCONF:
> + KERNEL_LOCK();
> NET_LOCK_SHARED();
> error = ifconf(data);
> NET_UNLOCK_SHARED();
> + KERNEL_UNLOCK();
> return (error);
> case SIOCIFGCLONERS:
> + KERNEL_LOCK();
> error = if_clone_list((struct if_clonereq *)data);
> + KERNEL_UNLOCK();
> return (error);
> case SIOCGIFGMEMB:
> + KERNEL_LOCK();
> NET_LOCK_SHARED();
> error = if_getgroupmembers(data);
> NET_UNLOCK_SHARED();
> + KERNEL_UNLOCK();
> return (error);
> case SIOCGIFGATTR:
> + KERNEL_LOCK();
> NET_LOCK_SHARED();
> error = if_getgroupattribs(data);
> NET_UNLOCK_SHARED();
> + KERNEL_UNLOCK();
> return (error);
> case SIOCGIFGLIST:
> + KERNEL_LOCK();
> NET_LOCK_SHARED();
> error = if_getgrouplist(data);
> NET_UNLOCK_SHARED();
> + KERNEL_UNLOCK();
> return (error);
> }
> +
> + KERNEL_LOCK();
>
> ifp = if_unit(ifr->ifr_name);
> if (ifp == NULL) {
>
