Re: ifconfig: print tpmr(4) members
> On 31 Jul 2020, at 17:17, Klemens Nanni wrote:
>
> This diff is to be applied on top of my other diff on tech@ with subject
> "ifconfig: merge switch_status() into bridge_status()".
>
> It hooks completes the output of tpmr intefaces in what I think is the
> simplest and least intrusive way.
>
> tpmr is a trivial bridge and has no specific ioctls, so to distinguish
> it from the rest we must rely on the interface name; assuming that it
> is tpmr because neither is_bridge() nor is_switch() return success is
> not possible due to the way ifconfig is designed: it runs all *_status()
> commands for all interface types.
>
> An alternative approach would be to make ifconfig try all the various
> bridge related ioctls on all bridge-like interfaces and quiet down all
> failures such output stays clean, but I dislike this shotgun approach
> and prefer testing for different drivers where possible.
>
> With this last piece in, I could finally document tpmr under ifconfig(8)
> (and move on the next drivers in need of love).
>
> Feedback? OK?
let's do it.
>
>
> --- brconfig.c.orig Fri Jul 31 08:58:03 2020
> +++ brconfig.cFri Jul 31 09:16:59 2020
> @@ -775,15 +775,28 @@
> return (1);
> }
>
> +/* no tpmr(4) specific ioctls, name is enough if ifconfig.c:printif() passed
> */
> +int
> +is_tpmr(void)
> +{
> + return (strncmp(ifname, "tpmr", sizeof("tpmr") - 1) == 0);
> +}
> +
> void
> bridge_status(void)
> {
> struct ifbrparam bp1, bp2;
> - int isswitch = is_switch();
> + int isswitch;
>
> + if (is_tpmr()) {
> + bridge_list("\t");
> + return;
> + }
> +
> if (!is_bridge())
> return;
>
> + isswitch = is_switch();
> if (isswitch)
> switch_cfg("\t");
> else
Re: switch: allow datapath_id and maxflow ioctls for non-root
> On 31 Jul 2020, at 14:28, Klemens Nanni wrote: > > ifconfig(8) detects switch(4) through its unique SIOCSWSDPID ioctl and > further does another switch specific ioctl for the default output > regardless of configuration and/or members: > > SIOCSWSDPID struct ifbrparam > Set the datapath_id in the OpenFlow protocol of the switch named > in ifbrp_name to the value in the ifbrpu_datapath field. > > SIOCSWGMAXFLOW struct ifbrparam > Retrieve the maximum number of flows in the OpenFlow protocol of > the switch named in ifbrp_name into the ifbrp_maxflow field. > > This is how it should look like: > > # ifconfig switch0 create > # ifconfig switch0 > switch0: flags=0<> > index 29 llprio 3 > groups: switch > datapath 0x5bea2b5b8e2456cf maxflow 1 maxgroup 1000 > > But using ifconfig as unprivileged user makes it fail switch(4) > interfaces as such and thus interprets them as bridge(4) instead: > > $ ifconfig switch0 > switch0: flags=0<> > index 29 llprio 3 > groups: switch > priority 32768 hellotime 2 fwddelay 15 maxage 20 holdcnt 6 > proto rstp > designated: id 00:00:00:00:00:00 priority 0 > > This is because the above mentioned ioctls are listed together with all > other bridge and switch related ioctls that set or write things. > Getting datapath_id and maxflow values however is read-only and crucial > for ifconfig as demonstrated above, so I'd like to move them out of the > root check to fix ifconfig. > > Feedback? OK? can't they be caught by the default case now? dlg > > > Index: sys/net/if.c > === > RCS file: /cvs/src/sys/net/if.c,v > retrieving revision 1.616 > diff -u -p -r1.616 if.c > --- sys/net/if.c 24 Jul 2020 18:17:14 - 1.616 > +++ sys/net/if.c 31 Jul 2020 04:13:40 - > @@ -2170,13 +2170,15 @@ ifioctl(struct socket *so, u_long cmd, c > case SIOCBRDGSIFCOST: > case SIOCBRDGSTXHC: > case SIOCBRDGSPROTO: > - case SIOCSWGDPID: > case SIOCSWSPORTNO: > - case SIOCSWGMAXFLOW: > #endif > if ((error = suser(p)) != 0) > break; > /* FALLTHROUGH */ > +#if NBRIDGE > 0 > + case SIOCSWGDPID: > + case SIOCSWGMAXFLOW: > +#endif > default: > error = ((*so->so_proto->pr_usrreq)(so, PRU_CONTROL, > (struct mbuf *) cmd, (struct mbuf *) data, >
Re: ifconfig: merge switch_status() into bridge_status()
> On 31 Jul 2020, at 16:36, Klemens Nanni wrote:
>
> On Wed, Jul 29, 2020 at 02:21:42PM +0200, Klemens Nanni wrote:
>> This is to reduce duplicate code and pave the way for a single
>> bridge_status() that covers all bridge like interfaces: bridge(4),
>> switch(4) and tpmr(4).
> A duplicate bridge_cfg() call snuck in, fixed diff below.
>
> Feedback? OK?
ok.
>
>> +if (isswitch)
>> +switch_cfg("\t");
>> +else
>> +bridge_cfg("\t");
>> +
>> bridge_cfg("\t");
>>
>> bridge_list("\t");
>
>
> Index: brconfig.c
> ===
> RCS file: /cvs/src/sbin/ifconfig/brconfig.c,v
> retrieving revision 1.26
> diff -u -p -r1.26 brconfig.c
> --- brconfig.c29 Jul 2020 12:13:28 - 1.26
> +++ brconfig.c31 Jul 2020 06:32:54 -
> @@ -54,6 +54,7 @@ void bridge_ifclrflag(const char *, u_in
>
> void bridge_list(char *);
> void bridge_cfg(const char *);
> +void switch_cfg(const char *);
> void bridge_badrule(int, char **, int);
> void bridge_showrule(struct ifbrlreq *);
> int is_switch(void);
> @@ -778,17 +779,24 @@ void
> bridge_status(void)
> {
> struct ifbrparam bp1, bp2;
> + int isswitch = is_switch();
>
> - if (!is_bridge() || is_switch())
> + if (!is_bridge())
> return;
>
> - bridge_cfg("\t");
> + if (isswitch)
> + switch_cfg("\t");
> + else
> + bridge_cfg("\t");
>
> bridge_list("\t");
>
> if (aflag && !ifaliases)
> return;
>
> + if (isswitch)
> + return;
> +
> strlcpy(bp1.ifbrp_name, ifname, sizeof(bp1.ifbrp_name));
> if (ioctl(sock, SIOCBRDGGCACHE, (caddr_t)) == -1)
> return;
> @@ -1146,8 +1154,8 @@ is_switch()
> return (1);
> }
>
> -static void
> -switch_cfg(char *delim)
> +void
> +switch_cfg(const char *delim)
> {
> struct ifbrparam bp;
>
> @@ -1168,20 +1176,6 @@ switch_cfg(char *delim)
> err(1, "%s", ifname);
>
> printf(" maxgroup %d\n", bp.ifbrp_maxgroup);
> -}
> -
> -void
> -switch_status(void)
> -{
> - if (!is_switch())
> - return;
> -
> - switch_cfg("\t");
> -
> - bridge_list("\t");
> -
> - if (aflag && !ifaliases)
> - return;
> }
>
> void
> Index: ifconfig.c
> ===
> RCS file: /cvs/src/sbin/ifconfig/ifconfig.c,v
> retrieving revision 1.424
> diff -u -p -r1.424 ifconfig.c
> --- ifconfig.c3 Jul 2020 17:42:50 - 1.424
> +++ ifconfig.c31 Jul 2020 06:24:17 -
> @@ -3507,7 +3507,6 @@ status(int link, struct sockaddr_dl *sdl
> phys_status(0);
> #ifndef SMALL
> bridge_status();
> - switch_status();
> #endif
> }
>
> Index: ifconfig.h
> ===
> RCS file: /cvs/src/sbin/ifconfig/ifconfig.h,v
> retrieving revision 1.2
> diff -u -p -r1.2 ifconfig.h
> --- ifconfig.h24 Oct 2019 18:54:10 - 1.2
> +++ ifconfig.h31 Jul 2020 06:24:18 -
> @@ -69,7 +69,6 @@ void bridge_flushrule(const char *, int)
> int is_bridge(void);
> void bridge_status(void);
> int bridge_rule(int, char **, int);
> -void switch_status(void);
> void switch_datapathid(const char *, int);
> void switch_portno(const char *, const char *);
>
Re: additions to unit(1)
On August 4, 2020 1:38:43 PM MDT, Tracey Emery wrote: >On August 4, 2020 1:24:18 PM MDT, Florian Obser >wrote: >>Because of reasons I recently had to carry a lot of garbage around for >>the municipality to pick up. They would only pickup 2 cubic meters in >>one sitting so I had to check how much I had. Turned out to be 0.9 >m^3. >> >>Of course inquisitive minds wanted to know how much that is in >>buttloads, to my great dismay units(1) did not know! >> >>So I had to do it by hand like some savage and of course I got the >>conversion wrong hence the following diff: >> >>You have: 0.9 m3 >>You want: buttloads >> * 0.53207987 >> / 1.8794171 >> >>About half a buttload, good to know. >> >>Extensive research on the subject[1] found definitions for the >>imperial buttload as well as the related metric assload. Curiously the >>assload is a measurement of mass while the buttload is a measurement >>of volume. >> >>Comments, OKs? >> >>diff --git units.lib units.lib >>index 0f811786bf3..df792a0a917 100644 >>--- units.lib >>+++ units.lib >>@@ -452,6 +452,7 @@ asb apostilb >> are 1e+2 m2 >> arpentcan27.52 mi >> arpentlin191.835 ft >>+assload 50 kg >> astronomicalunit AU >> atmosphere 1.01325e+5 nt/m2 >> atm atmosphere >>@@ -477,6 +478,8 @@ bolt 40 yd >> bottommeasure1|40 in >> Bq becquerel >> britishthermalunit 1.05506e+3 joule fuzz >>+butt 2 hogsheads >>+buttload 6 seams >> btu britishthermalunit >> refrigeration12000 btu/ton-hour >> buck dollar >> >>1) I tossed it into google and arrived at >>https://en.wiktionary.org/wiki/buttload >>as well as >>https://www.ed.ac.uk/files/imports/fileManager/donkey%20fact%20sheet.pdf >> >>-- >>I'm not entirely sure you are real. > >Baaaàhahahahahahaha! >-- >Tracey Emery Oh I forgot, you absolutely get an ok from me. Bahahahah, but seriously, Ok -- Tracey Emery
Re: additions to unit(1)
On August 4, 2020 1:24:18 PM MDT, Florian Obser wrote: >Because of reasons I recently had to carry a lot of garbage around for >the municipality to pick up. They would only pickup 2 cubic meters in >one sitting so I had to check how much I had. Turned out to be 0.9 m^3. > >Of course inquisitive minds wanted to know how much that is in >buttloads, to my great dismay units(1) did not know! > >So I had to do it by hand like some savage and of course I got the >conversion wrong hence the following diff: > >You have: 0.9 m3 >You want: buttloads > * 0.53207987 > / 1.8794171 > >About half a buttload, good to know. > >Extensive research on the subject[1] found definitions for the >imperial buttload as well as the related metric assload. Curiously the >assload is a measurement of mass while the buttload is a measurement >of volume. > >Comments, OKs? > >diff --git units.lib units.lib >index 0f811786bf3..df792a0a917 100644 >--- units.lib >+++ units.lib >@@ -452,6 +452,7 @@ asbapostilb > are 1e+2 m2 > arpentcan 27.52 mi > arpentlin 191.835 ft >+assload 50 kg > astronomicalunit AU > atmosphere1.01325e+5 nt/m2 > atm atmosphere >@@ -477,6 +478,8 @@ bolt 40 yd > bottommeasure 1|40 in > Bqbecquerel > britishthermalunit1.05506e+3 joule fuzz >+butt 2 hogsheads >+buttload 6 seams > btu britishthermalunit > refrigeration 12000 btu/ton-hour > buck dollar > >1) I tossed it into google and arrived at >https://en.wiktionary.org/wiki/buttload >as well as >https://www.ed.ac.uk/files/imports/fileManager/donkey%20fact%20sheet.pdf > >-- >I'm not entirely sure you are real. Baaaàhahahahahahaha! -- Tracey Emery
additions to unit(1)
Because of reasons I recently had to carry a lot of garbage around for the municipality to pick up. They would only pickup 2 cubic meters in one sitting so I had to check how much I had. Turned out to be 0.9 m^3. Of course inquisitive minds wanted to know how much that is in buttloads, to my great dismay units(1) did not know! So I had to do it by hand like some savage and of course I got the conversion wrong hence the following diff: You have: 0.9 m3 You want: buttloads * 0.53207987 / 1.8794171 About half a buttload, good to know. Extensive research on the subject[1] found definitions for the imperial buttload as well as the related metric assload. Curiously the assload is a measurement of mass while the buttload is a measurement of volume. Comments, OKs? diff --git units.lib units.lib index 0f811786bf3..df792a0a917 100644 --- units.lib +++ units.lib @@ -452,6 +452,7 @@ asb apostilb are1e+2 m2 arpentcan 27.52 mi arpentlin 191.835 ft +assload50 kg astronomicalunit AU atmosphere 1.01325e+5 nt/m2 atmatmosphere @@ -477,6 +478,8 @@ bolt40 yd bottommeasure 1|40 in Bq becquerel britishthermalunit 1.05506e+3 joule fuzz +butt 2 hogsheads +buttload 6 seams btubritishthermalunit refrigeration 12000 btu/ton-hour buck dollar 1) I tossed it into google and arrived at https://en.wiktionary.org/wiki/buttload as well as https://www.ed.ac.uk/files/imports/fileManager/donkey%20fact%20sheet.pdf -- I'm not entirely sure you are real.
Re: wsfontload(8): restore working default height/width values
On Tue, Jul 21, 2020 at 12:44:57PM +0200, Frederic Cambus wrote:
> Currently, wsfontload(8) hardcodes the default height and width values
> for the font to be loaded as 12x22 when using framebuffer consoles, and
> as 8x16 when in text mode. The 12x22 value wasn't correct in case we
> felt back to the smaller 8x16 font for screen widths smaller than 960px,
> and isn't valid anymore since we replaced Gallant 12x22 by Spleen 12x24
> on all architectures but sparc64.
>
> Here is a diff to get the default values for font height and width using
> the WSDISPLAYIO_GETSCREENTYPE ioctl. This ensures that they always match
> the currently loaded font metrics.
>
> Tested on a system with a text mode console only, on efifb(4) and
> inteldrm(4) framebuffer consoles, and on Loongson with smfb(4).
>
> Please note that this needs the diff I sent to tech@ yesterday to allow
> the WSDISPLAYIO_GETSCREENTYPE ioctl on tty*cfg.
>
> Comments? OK?
>
> Index: usr.sbin/wsfontload/wsfontload.c
> ===
> RCS file: /cvs/src/usr.sbin/wsfontload/wsfontload.c,v
> retrieving revision 1.22
> diff -u -p -r1.22 wsfontload.c
> --- usr.sbin/wsfontload/wsfontload.c 20 Jul 2020 14:09:24 - 1.22
> +++ usr.sbin/wsfontload/wsfontload.c 21 Jul 2020 10:41:56 -
> @@ -76,6 +76,7 @@ main(int argc, char *argv[])
> {
> char *wsdev, *infile, *p;
> struct wsdisplay_font f;
> + struct wsdisplay_screentype screens;
> int c, res, wsfd, ffd, type, list, i;
> int defwidth, defheight;
> struct stat stat;
> @@ -178,23 +179,16 @@ main(int argc, char *argv[])
> ffd = STDIN_FILENO;
> }
>
> - res = ioctl(wsfd, WSDISPLAYIO_GTYPE, );
> - if (res != 0)
> - type = WSDISPLAY_TYPE_UNKNOWN;
> -
> - switch (type) {
> - /* text-mode VGA */
> - case WSDISPLAY_TYPE_ISAVGA:
> - case WSDISPLAY_TYPE_PCIVGA:
> + memset(, 0, sizeof(screens));
> + res = ioctl(wsfd, WSDISPLAYIO_GETSCREENTYPE, );
> + if (res == 0) {
> + /* raster frame buffers */
> + defwidth = screens.fontwidth;
> + defheight = screens.fontheight;
> + } else {
> + /* text-mode VGA */
> defwidth = 8;
> defheight = 16;
> - break;
> - /* raster frame buffers */
> - default:
> - /* XXX ought to be computed from the frame buffer resolution */
> - defwidth = 12;
> - defheight = 22;
> - break;
> }
>
> f.index = -1;
Ping. Anyone willing to look at this? Thanks!
Re: Allow the WSDISPLAYIO_GETSCREENTYPE ioctl on tty*cfg
On Mon, Jul 20, 2020 at 04:36:46PM +0200, Frederic Cambus wrote:
> Here is a diff to allow the WSDISPLAYIO_GETSCREENTYPE ioctl on the
> tty*cfg device, passing it back to tty*0.
>
> I need this to restore working defaults in wsfontload(8).
>
> Comments? OK?
>
> Index: sys/dev/wscons/wsdisplay.c
> ===
> RCS file: /cvs/src/sys/dev/wscons/wsdisplay.c,v
> retrieving revision 1.141
> diff -u -p -r1.141 wsdisplay.c
> --- sys/dev/wscons/wsdisplay.c25 May 2020 09:55:49 - 1.141
> +++ sys/dev/wscons/wsdisplay.c20 Jul 2020 14:28:42 -
> @@ -1046,10 +1046,15 @@ wsdisplayioctl(dev_t dev, u_long cmd, ca
> #endif
>
> if (ISWSDISPLAYCTL(dev)) {
> - if (cmd != WSDISPLAYIO_GTYPE)
> + switch (cmd) {
> + case WSDISPLAYIO_GTYPE:
> + case WSDISPLAYIO_GETSCREENTYPE:
> + /* pass to the first screen */
> + dev = makedev(major(dev), WSDISPLAYMINOR(unit, 0));
> + break;
> + default:
> return (wsdisplay_cfg_ioctl(sc, cmd, data, flag, p));
> - /* pass WSDISPLAYIO_GTYPE to the first screen */
> - dev = makedev(major(dev), WSDISPLAYMINOR(unit, 0));
> + }
> }
>
> if (WSDISPLAYSCREEN(dev) >= WSDISPLAY_MAXSCREEN)
Ping. Anyone willing to look at this? Thanks!
lex: reset yy_at_bol in yyless() if yyleng is non-zero.
Fixes a bug where calling yyless() to unput all the chars before a newline would not reset the beginning of line marker. Otherwise, flex won't match rules that start with a caret (^) after calling yyless() in thise case. AT lex behaves correctly. Upstream PR: https://github.com/westes/flex/pull/452 - todd Index: usr.bin/lex/flex.skl === RCS file: /cvs/src/usr.bin/lex/flex.skl,v retrieving revision 1.16 diff -u -p -u -r1.16 flex.skl --- usr.bin/lex/flex.skl2 May 2017 19:16:19 - 1.16 +++ usr.bin/lex/flex.skl4 Aug 2020 16:01:22 - @@ -532,6 +532,9 @@ m4_ifdef( [[M4_YY_NOT_IN_HEADER]], YY_RESTORE_YY_MORE_OFFSET \ YY_G(yy_c_buf_p) = yy_cp = yy_bp + yyless_macro_arg - YY_MORE_ADJ; \ YY_DO_BEFORE_ACTION; /* set up yytext again */ \ + if ( yyleng > 0 ) \ + YY_CURRENT_BUFFER_LVALUE->yy_at_bol = \ + (yytext[yyleng - 1] == '\n'); \ } \ while ( 0 ) ]])
Re: NET_LOCK and trunk detach
On Tue, Jul 28, 2020 at 5:27 PM Vitaliy Makkoveev wrote: > > > > > On 29 Jul 2020, at 00:09, sven falempin wrote: > > > > On Tue, Jul 28, 2020 at 4:42 PM Vitaliy Makkoveev wrote: > >> > >> On Tue, Jul 28, 2020 at 04:10:01PM -0400, sven falempin wrote: > >>> Hello, > >>> > >>> I am running some trunk interfaces in a multi core environment, > >>> it's a slightly modified version, i have a few NET_ASSERT_LOCKED(); > >>> suspecting some multi core shenanigans, which i guess was confirmed: > >>> (unsure the have X meaning, but i ' m pretty sure 256 is very wrong) > >>> the if_trunk.c locking is completely unmodified > >>> The code is 6.7-stable > >>> > >>> splassert: lacp_detach: want 2 have 0 > >>> splassert: lacp_detach: want 2 have 0 > >>> splassert: lacp_detach: want 2 have 256 > >>> > >>> I noticed : trunk_clone_destroy ,call > >>> > >>>if (tr->tr_proto != TRUNK_PROTO_NONE) > >>>tr->tr_detach(tr); > >>> > >>> outside the lock, and that trunk_ioctl call it > >>> > >>>if (tr->tr_proto != TRUNK_PROTO_NONE) > >>>error = tr->tr_detach(tr); > >>> > >>> but ioctl is as far as i understand locked. > >>> I'm unsure if the difficult and amazing unlocking work > >>> did an oopsies or if ioctl is already assumed unlocked. > >>> > >>> Kindly inform me. > >>> Best regards, thank you for reading. > >>> > >> > >> lacp_detach() touches nothing which requires NET_LOCK(). What is the > >> reason you placed assertion to lacp_detach()? > > > > <> > > > > the lacp_detach is not yours, i put them here because i have a NULL pointer > > popping in other 'driver' callback. > > > > I'm tracking this and trying to understand how this memory is 'nullified' > > mid function. > > I have no telepathy skills. Sorry. If you have panics send please dmesg(8) > output and instruction for reproduce. > > > > > I do not think putting NET_ASSERT_LOCKED can be harmful in any way. > > If so please tell me. > > > > What is the data you think needs be protected by netlock? > > > I am just tracking a bug and noticed these detach locking strangeness. > > > After further analysis the *current* conclusion is: * current code is sort of safe, but the delete would be safer with a lock ( for consistency anyway ) * if the code run into a VM that do strange stuff with the stack it creates bugs ( the bug only appears in KVM cpu , a qemu emulating a real cpu does not have this problem ) -- -- - Knowing is not enough; we must apply. Willing is not enough; we must do
Re: pipex(4): kill pipexintr()
On Tue, Aug 04, 2020 at 01:53:55PM +0900, YASUOKA Masahiko wrote:
> On Mon, 3 Aug 2020 23:36:09 +0300
> Vitaliy Makkoveev wrote:
> > On Tue, Aug 04, 2020 at 01:26:14AM +0900, YASUOKA Masahiko wrote:
> >> Comments?
> >
> > You introduce `cookie' as
> >
> > cookie = session->protocol << 16 | session->session_id;
> >
> > also multicast sessions initialized as
> >
> > session->protocol = PIPEX_PROTO_NONE;
> > session->session_id = ifindex;
> >
> > `protocol' and `session_id' come from userland, so I like to have checks
> > like below. It's allow us to avoid `cookie' be broken while
> > `pr_session_id' exceeds 16 bit integer. Also userland should not pass
> > PIPEX_PROTO_NONE as `pr_protocol' because we shouldn't have multicast
> > and not multicast sessions with the same `cookie'.
> >
> > cut begin
> >
> > pipex_init_session(struct pipex_session **rsession,
> > struct pipex_session_req *req)
> > {
> > if (req->pr_protocol == PIPEX_PROTO_NONE)
> > return (EINVAL);
>
> pipex_init_session() has the same check already.
My fault. Sorry.
>
> 287 int
> 288 pipex_init_session(struct pipex_session **rsession,
> 289 struct pipex_session_req *req)
> 290 {
> (snip)
> 297 switch (req->pr_protocol) {
> 298 #ifdef PIPEX_PPPOE
> 299 case PIPEX_PROTO_PPPOE:
> (snip)
> 333 default:
> 334 return (EPROTONOSUPPORT);
> 335 }
>
> >
> > if (req->pr_session_id > 0x)
> > return (EINVAL);
> >
> > cut end
>
> req->pr_session_id can't be > 0x since it's uint16_t.
>
> > Also cookies introduce invalidation problem. Yes, it has low
> > probability, but we can have operation order like below:
> >
> > 1. enqueue session with `protocol' = 0xaa and `session_id' = 0xbb, and
> > `cookie' = 0xaabb
> > 2. kill this session
> > 3. create new session `protocol' = 0xaa and `session_id' = 0xbb
> > 4. this newly created session will be used by pipexintr()
> >
> > As I have seen while played with refcounters, session can be enqueued
> > more than 10 times...
>
> The diff makes the problem worse, but it could happen already if the
> session-id is reused.
>
> > Also It's not obvious that interface index will never exceed 16 bit
> > counter. It's unsigned int and may be underlay counter's resolution
> > will be expanded in future. So I like to have at least corresponding
> > assertion in pipex_iface_init().
>
> Right. This is fixable with another unique number.
>
> > So, may be my first solution is the best here. And, as mpi@ pointed,
> > ipsec(4) should be reworked to allow parallelism.
>
> Does first mean killing the pipexintr?
Yes.
>
> What I explained was wrong. I'm sorry about this.
>
> On Fri, 31 Jul 2020 09:36:32 +0900 (JST)
> YASUOKA Masahiko wrote:
> > A packet of L2TP/IPsec (encapsulated IP/PPP/L2TP/UDP/ESP/UDP/IP) is
> > processed like:
> >
> >ipv4_input
> > ...
> >udp_input
> > ipsec_common_input
> > esp_input
> >crypto_dispatch
> > => crypto_taskq_mp_safe
> >
> >kthread "crynlk"
> > crypto_invoke
> >... (*1)
> > crypto_done
> > esp_input_cb
> >ipsec_common_input_cb
> > ip_deliver
> >udp_input
> > pipex_l2tp_input
> >pipex_common_input
> > (*2)
> > pipex_ppp_input
> >pipex_mppe_input (*3)
> > pipex_ppp_input
> >pipex_ip_input
> > ipv4_input
> >...
>
> This should be
>
>kthread "crynlk"
> crypto_invoke
>... (*1)
> crypto_done
>kthread "crypto" < another thread
> ipsec_input_cb < this is missed
>esp_input_cb
> ipsec_common_input_cb
>ip_deliver
> udp_input
>pipex_l2tp_input
> pipex_common_input
>(*2)
>pipex_ppp_input
> pipex_mppe_input (*3)
>pipex_ppp_input
> pipex_ip_input
>ipv4_input
> ...
>
> > At *2 there was a queue. "crynlk" is a busy thread, since it is doing
> > decryption at *1. I think it's better pipex input is be done by
> > another thread than crypto since it also has decryption at *3.
>
> This is false. *3 is done by another thread.
> It is the same if crypto driver is not CRYPTOCAP_F_MPSAFE.
> (crypto_invoke() is done by the caller's thread and the callback
> (ipsec_input_cb) is called by"crypto" thread.)
>
> So I have no actual reason to keep the queues.
>
> ok yasuoka for the diff which kills pipexintr.
>
Thanks for explanation and reviews.
Re: [PATCH] pipex(4): rework PPP input
Sorry for delayed reply. On Wed, 27 May 2020 01:29:36 +0300 Sergey Ryazanov wrote: > On Tue, May 26, 2020 at 12:07 PM Vitaliy Makkoveev > wrote: >>> On 25 May 2020, at 22:04, Sergey Ryazanov wrote: >>> On Sat, May 23, 2020 at 3:07 PM Vitaliy Makkoveev >>> wrote: For example, each pipex session should have unique pair of `protocol’ and `session_id’. These values are passed from userland. While the only instance of npppd(8) uses pipex(4) this is not the problem. But you introduce the case while pipex(4) will be used by multiple independent userland programs. At least, I have interest how you handle this. >>> >>> This should not be a problem here. npppd(8) support server mode only. >>> While my work is to implement acceleration for client side of L2TP >>> connection. >> >> I guess they can coexist. Also you can have multiple connections to >> ppp servers simultaneously. > > With 16 bits long session id field, according to birthday problem to > reach 0.9 collision probability I need 549 simultaneous sessions. > Should I still be worried or I have a time to complete integration > work and then update UDP filter for love of the game? usr.sbin/npppd/l2tp/l2tp_local.h 79 #define L2TP_SESSION_ID_MASK0x7fff npppd uses 0-32767
