Re: push kernel lock inside ifioctl_get()

2022-11-08 Thread Klemens Nanni 
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()

2022-11-08 Thread Vitaliy Makkoveev 
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()

2022-11-08 Thread Klemens Nanni 
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()

2022-11-08 Thread Klemens Nanni 
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()

2022-11-08 Thread Vitaliy Makkoveev 
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()

2022-11-08 Thread Martin Pieuchot 
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) {
>

push kernel lock inside ifioctl_get()

2022-11-08 Thread Klemens Nanni 
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.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) {

push kernel lock into ifioctl_get()

2022-11-08 Thread Klemens Nanni 
Another mechanical diff without semantic changes to avoid churn in
actual unlocking diffs.

OK?

Index: if.c
===
RCS file: /cvs/src/sys/net/if.c,v
retrieving revision 1.666
diff -u -p -r1.666 if.c
--- if.c8 Nov 2022 11:25:01 -   1.666
+++ if.c8 Nov 2022 13:53:27 -
@@ -1979,9 +1979,7 @@ ifioctl(struct socket *so, u_long cmd, c
case SIOCGIFRDOMAIN:
case SIOCGIFGROUP:
case SIOCGIFLLPRIO:
-   KERNEL_LOCK();
error = ifioctl_get(cmd, data);
-   KERNEL_UNLOCK();
return (error);
}
 
@@ -2428,6 +2426,8 @@ ifioctl_get(u_long cmd, caddr_t data)
size_t bytesdone;
const char *label;
 
+   KERNEL_LOCK();
+
switch(cmd) {
case SIOCGIFCONF:
NET_LOCK_SHARED();
@@ -2455,8 +2455,10 @@ ifioctl_get(u_long cmd, caddr_t data)
}
 
ifp = if_unit(ifr->ifr_name);
-   if (ifp == NULL)
+   if (ifp == NULL) {
+   KERNEL_UNLOCK();
return (ENXIO);
+   }
 
NET_LOCK_SHARED();
 
@@ -2527,6 +2529,8 @@ ifioctl_get(u_long cmd, caddr_t data)
}
 
NET_UNLOCK_SHARED();
+
+   KERNEL_UNLOCK();
 
if_put(ifp);