Re: RFC: ipsec(4) pseudo interface

[Kengo NAKAHARA]

Hi,

On 2017/12/23 9:05, Christos Zoulas wrote:
> In article ,
> Kengo NAKAHARA   wrote:
>> Hi,
>>
>> Thank you for your reviewing.
> 
> 
> Thanks for fixing; more nit-picking:
> 1. there is a variable called err instead of error why (all the rest
>are called error)?

Oh, good catch. I fix it(ipsecif4_fragout()).

> 2. I prefer fewer lines of code, fewer variables for all the copies
>of those similar functions, like:
> 
> +static int
> +if_ipsec_encap_detach(struct ipsec_variant *var)
> +{
> +
> + KASSERT(var != NULL);
> + KASSERT(if_ipsec_variant_is_configured(var));
> +
> + switch (var->iv_psrc->sa_family) {
> +#ifdef INET
> + case AF_INET:
> + return ipsecif4_detach(var);
> +#endif /* INET */
> +#ifdef INET6
> + case AF_INET6:
> + return ipsecif6_detach(var);
> + break;
> +#endif /* INET6 */
> + default:
> + return EINVAL;
> + }
> +}

Exactly. I fix the following duplications.
+ if_ipsec_encap_attach() and if_ipsec_encap_detach()
+ if_ipsec_gather_addr_port() and if_ipsec_set_addr_port()
  - remove if_ipsec_gather_addr_port()
+ sa_len checking in if_ipsec_ioctl()
  - functionize it as if_ipsec_check_salen()

Here is the updated patch series and unified patch.
- https://www.netbsd.org/~knakahara/if_ipsec/if_ipsec4.tgz
- https://www.netbsd.org/~knakahara/if_ipsec/if_ipsec4-unified.patch


Thanks,

-- 
//
Internet Initiative Japan Inc.

Device Engineering Section,
IoT Platform Development Department,
Network Division,
Technology Unit

Kengo NAKAHARA 

xcall while cold == 1

[Masanobu SAITOH]

 Hi.

 While debugging PR#52820 ("boot -1" panics on systems with ixgX interfaces),
I've noticed that xcall doesn't work while cold == 1.

 When I added softint_disestablish() near the end of the ixgbe_attach().
The following panic occured:


panic: kernel diagnostic assertion "xc->xc_donep < xc->xc_headp" failed: file 
"../../../../kern/subr_xcall.c", line 278


This KASSERT is:


static inline uint64_t
xc_lowpri(xcfunc_t func, void *arg1, void *arg2, struct cpu_info *ci)
{
xc_state_t *xc = _low_pri;
CPU_INFO_ITERATOR cii;
uint64_t where;

mutex_enter(>xc_lock);
while (xc->xc_headp != xc->xc_donep) {
cv_wait(>xc_busy, >xc_lock);
}
xc->xc_arg1 = arg1;
xc->xc_arg2 = arg2;
xc->xc_func = func;
if (ci == NULL) {
xc_broadcast_ev.ev_count++;
for (CPU_INFO_FOREACH(cii, ci)) {
if ((ci->ci_schedstate.spc_flags & SPCF_RUNNING) == 0)
continue;
xc->xc_headp += 1;
ci->ci_data.cpu_xcall_pending = true;
cv_signal(>ci_data.cpu_xcall);
}
} else {
xc_unicast_ev.ev_count++;
xc->xc_headp += 1;
ci->ci_data.cpu_xcall_pending = true;
cv_signal(>ci_data.cpu_xcall);
}
KASSERT(xc->xc_donep < xc->xc_headp);<= Here!
where = xc->xc_headp;
mutex_exit(>xc_lock);

/* Return a low priority ticket. */
KASSERT((where & XC_PRI_BIT) == 0);
return where;
}


So I added the following debug printf:
-
@@ -252,6 +253,7 @@ xc_lowpri(xcfunc_t func, void *arg1, voi
xc_state_t *xc = _low_pri;
CPU_INFO_ITERATOR cii;
uint64_t where;
+   int i = -1;

mutex_enter(>xc_lock);
while (xc->xc_headp != xc->xc_donep) {
@@ -263,8 +265,11 @@ xc_lowpri(xcfunc_t func, void *arg1, voi
if (ci == NULL) {
xc_broadcast_ev.ev_count++;
for (CPU_INFO_FOREACH(cii, ci)) {
-   if ((ci->ci_schedstate.spc_flags & SPCF_RUNNING) == 0)
+   i++;
+   if ((ci->ci_schedstate.spc_flags & SPCF_RUNNING) == 0) {
+   printf("cpu %d: XXX not running\n", i);
continue;
+   }
xc->xc_headp += 1;
ci->ci_data.cpu_xcall_pending = true;
cv_signal(>ci_data.cpu_xcall);
@@ -275,6 +280,9 @@ xc_lowpri(xcfunc_t func, void *arg1, voi
ci->ci_data.cpu_xcall_pending = true;
cv_signal(>ci_data.cpu_xcall);
}
+   if (xc->xc_donep >= xc->xc_headp)
+   printf("XXX donep = %" PRIu64 ", headp = %" PRIu64
+   ", ci = %p\n", xc->xc_donep, xc->xc_headp, ci);
KASSERT(xc->xc_donep < xc->xc_headp);
where = xc->xc_headp;
mutex_exit(>xc_lock);
-

The output says

cpu 0: XXX not running
cpu 1: XXX not running
cpu 2: XXX not running
cpu 3: XXX not running
XXX donep = 0, headp = 0, ci = 0x0
panic: kernel diagnostic assertion "xc->xc_donep < xc->xc_headp" failed: file 
"../../../../kern/subr_xcall.c", line 286


(Yes, the exact reason is not cold==1 but all CPU's SPCF_RUNNING is not set)


 Is this intended behavior? Is it possible to use xcall while
cold==1?

For softint_establish(), the following diff avoid panic:

Index: kern_softint.c
===
RCS file: /cvsroot/src/sys/kern/kern_softint.c,v
retrieving revision 1.44
diff -u -p -r1.44 kern_softint.c
--- kern_softint.c  22 Nov 2017 02:20:21 -  1.44
+++ kern_softint.c  25 Dec 2017 06:31:57 -
@@ -177,6 +177,7 @@ __KERNEL_RCSID(0, "$NetBSD: kern_softint
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -430,8 +431,10 @@ softint_disestablish(void *arg)
 * it again.  So, we are only looking for handler records with
 * SOFTINT_ACTIVE already set.
 */
-   where = xc_broadcast(0, (xcfunc_t)nullop, NULL, NULL);
-   xc_wait(where);
+   if (__predict_true(cold == 0)) {
+   where = xc_broadcast(0, (xcfunc_t)nullop, NULL, NULL);
+   xc_wait(where);
+   }

for (;;) {
/* Collect flag values from each CPU. */


 I don't know whether this is good fix or not.

 One of the workaround is not call softint_disesablish() in
xxx_attach() and defer it using with config_interrupts(), but
I think it's dirty.

 I'm now writing a code for ixg(4) to fallback from MSI-X to
legacy interrupt when resource 

Re: Proposal to obsolete SYS_pipe

[Robert Elz]

Date:Sun, 24 Dec 2017 18:42:19 -0800
From:John Nemeth 
Message-ID:  <201712250242.vbp2gjjm017...@server.cornerstoneservice.ca>

  | HISTORY
  |  A pipe() function call appeared in Version 6 AT UNIX.

That I think would be a man page bug - pipe() was certainly in 5th
edition, but that is as far back as I go, so I am not sure when it
did appear - the syscall number suggests it was not in the very early
versions though (not 1st or 2nd edition probably.)

kre

Re: Proposal to obsolete SYS_pipe

[Robert Elz]

Date:Sun, 24 Dec 2017 22:25:15 +0100
From:Kamil Rytarowski 
Message-ID:  <88ef8f04-bd40-af2d-6284-eb0376895...@gmail.com>

  |  - I've marked pipe(2) as compat_80.

Because we would need this (obviously) there is nothing really being
gained by "removing" the syscall, as it is not really being removed,
just moved, and consequently I see no benefit worthy of the churn - it
all just looks like busy work.

kre

Re: Proposal to obsolete SYS_pipe

[Mouse]

> My NetBSD 7.x systems have the manpage as well.  One might wish to
> look for manpages on a system newer then 1.4T.  :->

5.2, slightly newer than 1.4T :), doesn't have it.

I would argue that it still needs pipe(2) to be converted into pipe(3)
with an xref to pipe2(2).

> The big thing is that I don't see what the difference[s] between
> pipe(2) and pipe2(2) are, other then that pipe2(2) takes an extra
> flags argument, i.e. I don't see how it solves the problem stated in
> the original message.

I'm guessing here, but my guess is that the syscall ABI (ie, the
interface at the kernel/user boundary) takes a pointer-to-two-ints (a
la socketpair) rather than expecting to get the ints via two return
values from the syscall.

That looks like one difference, from the diffs, and it also is pretty
much exactly what the stated problem is.

/~\ The ASCII Mouse
\ / Ribbon Campaign
 X  Against HTMLmo...@rodents-montreal.org
/ \ Email!   7D C8 61 52 5D E7 2D 39  4E F1 31 3E E8 B3 27 4B

Re: Proposal to obsolete SYS_pipe

[John Nemeth]

On Dec 24,  9:37pm, Mouse wrote:
}
} > http://netbsd.org/~kamil/patch-00039-obsolete-SYS_pipe.txt
} 
} I see no pipe2(2), nor change from pipe(2) to pipe(3) (with an xref to
} pipe2(2)), both of which, it seems to me, should be part of this.

 From: http://netbsd.gw.com/cgi-bin/man-cgi?pipe2+2+NetBSD-current

HISTORY
 A pipe() function call appeared in Version 6 AT UNIX.  The pipe2()
 function is inspired from Linux and appeared in NetBSD 6.0.

My NetBSD 7.x systems have the manpage as well.  One might wish to
look for manpages on a system newer then 1.4T.  :->

 The big thing is that I don't see what the difference between
pipe(2) and pipe2(2) are, other then that pipe2(2) takes an extra
flags argument, i.e. I don't see how it solves the problem stated
in the original message.

}-- End of excerpt from Mouse

Re: Proposal to obsolete SYS_pipe

[Mouse]

> http://netbsd.org/~kamil/patch-00039-obsolete-SYS_pipe.txt

I see no pipe2(2), nor change from pipe(2) to pipe(3) (with an xref to
pipe2(2)), both of which, it seems to me, should be part of this.

/~\ The ASCII Mouse
\ / Ribbon Campaign
 X  Against HTMLmo...@rodents-montreal.org
/ \ Email!   7D C8 61 52 5D E7 2D 39  4E F1 31 3E E8 B3 27 4B

Re: Proposal to obsolete SYS_pipe

[Joerg Sonnenberger]

On Sun, Dec 24, 2017 at 10:25:15PM +0100, Kamil Rytarowski wrote:
> It is a special syscall that returns two integers from one function
> call. Fanciness is not compatible with regular C syntax and it demands
> per-cpu assembly wrappers and rump-kernel workarounds. It's not easily
> usable with syscall(2).

So far I see no good reason for this move. I consider the very existance
of syscall(2) a bug, so any justification based on "it can't be used
with it" is hallow.

Joerg

Proposal to obsolete SYS_pipe

[Kamil Rytarowski]

I propose to deprecate SYS_pipe.

It is a special syscall that returns two integers from one function
call. Fanciness is not compatible with regular C syntax and it demands
per-cpu assembly wrappers and rump-kernel workarounds. It's not easily
usable with syscall(2).

OpenBSD and FreeBSD already deprecated this traditional pipe(2) syscall
replacing it with a more portable equivalent in C.

Changes:
 - I've marked pipe(2) as compat_80.
 - I've removed generation of garbage in retval[2] from pipe2(2).
 - I've reimplemented in C the pipe(2) syscall with pipe2(2).
 - I've adjusted the surrounding code for the changes.

http://netbsd.org/~kamil/patch-00039-obsolete-SYS_pipe.txt