Re: Simplify NET_LOCK() variations

2020-04-12 Thread Alexandr Nedvedicky
Hallo,

On Sun, Apr 12, 2020 at 07:02:43PM +0200, Mark Kettenis wrote:
> > From: "Theo de Raadt" 
> > Date: Sun, 12 Apr 2020 10:28:59 -0600
> > 
> > > + if ((p->p_flag & P_SYSTEM) &&
> > > + (strncmp(p->p_p->ps_comm, "softnet", 7) == 0))
> > 
> > Wow that is ugly.  
> 
> A better approach might be to store a pointer to the softnet task's
> struct proc in a global variable and check that.  That is what we do
> for the pagedaemon for example.
> 

I'm not sure same thing would work for network task. Currently
there is a single instance of pagedaemon, however we hope to
have more network tasks running in parallel.

How about sticking a magic flag to proc and task? The idea is just
shown in diff below. Just idea I have not tried to compile it.

regards
sashan

8<---8<---8<--8<
diff --git a/sys/kern/kern_task.c b/sys/kern/kern_task.c
index 5ed4e3a7c39..0698041c3aa 100644
--- a/sys/kern/kern_task.c
+++ b/sys/kern/kern_task.c
@@ -53,6 +53,7 @@ struct taskq {
 #ifdef WITNESS
struct lock_object   tq_lock_object;
 #endif
+   int  tq_magic;
 };
 
 static const char taskq_sys_name[] = "systq";
@@ -129,6 +130,7 @@ taskq_create(const char *name, unsigned int nthreads, int 
ipl,
tq->tq_nthreads = nthreads;
tq->tq_name = name;
tq->tq_flags = flags;
+   tq->tq_magic = 0;
 
mtx_init_flags(>tq_mtx, ipl, name, 0);
TAILQ_INIT(>tq_worklist);
@@ -146,6 +148,18 @@ taskq_create(const char *name, unsigned int nthreads, int 
ipl,
return (tq);
 }
 
+struct taskq *
+taskq_create_magic(const char *name, unsigned int nthreads, int ipl,
+unsigned int flags, int magic)
+{
+   struct taskq *tq;
+
+   tq = taskq_create(nmae, nthreads, ipl, flags);
+   if (tq != NULL)
+   tq->tq_magic = magic;
+   return (tq);
+}
+
 void
 taskq_destroy(struct taskq *tq)
 {
@@ -364,8 +378,11 @@ taskq_thread(void *xtq)
WITNESS_CHECKORDER(>tq_lock_object, LOP_NEWORDER, NULL);
 
while (taskq_next_work(tq, )) {
+   struct proc *p = curproc;
WITNESS_LOCK(>tq_lock_object, 0);
+   p->p_magic = tq->tq_magic;
(*work.t_func)(work.t_arg);
+   p->p_magic = 0;
WITNESS_UNLOCK(>tq_lock_object, 0);
sched_pause(yield);
}
diff --git a/sys/net/if.c b/sys/net/if.c
index 176fcf849fd..442a7bcec9a 100644
--- a/sys/net/if.c
+++ b/sys/net/if.c
@@ -266,7 +266,8 @@ ifinit(void)
timeout_set(_tick_to, net_tick, _tick_to);
 
for (i = 0; i < NET_TASKQ; i++) {
-   nettqmp[i] = taskq_create("softnet", 1, IPL_NET, TASKQ_MPSAFE);
+   nettqmp[i] = taskq_create_magic("softnet", 1, IPL_NET,
+   TASKQ_MPSAFE, 0x0030fd7ed); /* SOFTNET task */
if (nettqmp[i] == NULL)
panic("unable to create network taskq %d", i);
}
diff --git a/sys/sys/proc.h b/sys/sys/proc.h
index a25cb52951d..51ac96da5cc 100644
--- a/sys/sys/proc.h
+++ b/sys/sys/proc.h
@@ -408,6 +408,7 @@ struct proc {
union sigval p_sigval;  /* For core dump/debugger XXX */
longp_sitrapno; /* For core dump/debugger XXX */
int p_sicode;   /* For core dump/debugger XXX */
+   int p_magic;/* Identifies process context (for asserts) */
 };
 
 /* Status values. */



Re: slaacd(8): honour rdomain we are running in

2020-04-12 Thread Sebastian Benoit
Florian Obser(flor...@openbsd.org) on 2020.04.12 19:53:23 +0200:
> OK?

yes, this is probably better than having it configurable via option.

> diff --git slaacd.c slaacd.c
> index 58f15bcda37..dae2eab3434 100644
> --- slaacd.c
> +++ slaacd.c
> @@ -755,7 +755,7 @@ configure_gateway(struct imsg_configure_dfr *dfr, uint8_t 
> rtm_type)
>   rtm.rtm_version = RTM_VERSION;
>   rtm.rtm_type = rtm_type;
>   rtm.rtm_msglen = sizeof(rtm);
> - rtm.rtm_tableid = 0; /* XXX imsg->rdomain; */
> + rtm.rtm_tableid = getrtable();
>   rtm.rtm_index = dfr->if_index;
>   rtm.rtm_seq = ++rtm_seq;
>   rtm.rtm_priority = RTP_NONE;
> @@ -852,7 +852,7 @@ send_rdns_proposal(struct imsg_propose_rdns *rdns)
>   rtm.rtm_version = RTM_VERSION;
>   rtm.rtm_type = RTM_PROPOSAL;
>   rtm.rtm_msglen = sizeof(rtm);
> - rtm.rtm_tableid = 0; /* XXX imsg->rdomain; */
> + rtm.rtm_tableid = getrtable();
>   rtm.rtm_index = rdns->if_index;
>   rtm.rtm_seq = ++rtm_seq;
>   rtm.rtm_priority = RTP_PROPOSAL_SLAAC;
> 
> 
> -- 
> I'm not entirely sure you are real.
> 



Re: slaacd(8): honour rdomain we are running in

2020-04-12 Thread Denis Fondras
On Sun, Apr 12, 2020 at 07:53:23PM +0200, Florian Obser wrote:
> OK?
> 

OK denis@

> diff --git slaacd.c slaacd.c
> index 58f15bcda37..dae2eab3434 100644
> --- slaacd.c
> +++ slaacd.c
> @@ -755,7 +755,7 @@ configure_gateway(struct imsg_configure_dfr *dfr, uint8_t 
> rtm_type)
>   rtm.rtm_version = RTM_VERSION;
>   rtm.rtm_type = rtm_type;
>   rtm.rtm_msglen = sizeof(rtm);
> - rtm.rtm_tableid = 0; /* XXX imsg->rdomain; */
> + rtm.rtm_tableid = getrtable();
>   rtm.rtm_index = dfr->if_index;
>   rtm.rtm_seq = ++rtm_seq;
>   rtm.rtm_priority = RTP_NONE;
> @@ -852,7 +852,7 @@ send_rdns_proposal(struct imsg_propose_rdns *rdns)
>   rtm.rtm_version = RTM_VERSION;
>   rtm.rtm_type = RTM_PROPOSAL;
>   rtm.rtm_msglen = sizeof(rtm);
> - rtm.rtm_tableid = 0; /* XXX imsg->rdomain; */
> + rtm.rtm_tableid = getrtable();
>   rtm.rtm_index = rdns->if_index;
>   rtm.rtm_seq = ++rtm_seq;
>   rtm.rtm_priority = RTP_PROPOSAL_SLAAC;
> 
> 
> -- 
> I'm not entirely sure you are real.
> 



slaacd(8): honour rdomain we are running in

2020-04-12 Thread Florian Obser
OK?

diff --git slaacd.c slaacd.c
index 58f15bcda37..dae2eab3434 100644
--- slaacd.c
+++ slaacd.c
@@ -755,7 +755,7 @@ configure_gateway(struct imsg_configure_dfr *dfr, uint8_t 
rtm_type)
rtm.rtm_version = RTM_VERSION;
rtm.rtm_type = rtm_type;
rtm.rtm_msglen = sizeof(rtm);
-   rtm.rtm_tableid = 0; /* XXX imsg->rdomain; */
+   rtm.rtm_tableid = getrtable();
rtm.rtm_index = dfr->if_index;
rtm.rtm_seq = ++rtm_seq;
rtm.rtm_priority = RTP_NONE;
@@ -852,7 +852,7 @@ send_rdns_proposal(struct imsg_propose_rdns *rdns)
rtm.rtm_version = RTM_VERSION;
rtm.rtm_type = RTM_PROPOSAL;
rtm.rtm_msglen = sizeof(rtm);
-   rtm.rtm_tableid = 0; /* XXX imsg->rdomain; */
+   rtm.rtm_tableid = getrtable();
rtm.rtm_index = rdns->if_index;
rtm.rtm_seq = ++rtm_seq;
rtm.rtm_priority = RTP_PROPOSAL_SLAAC;


-- 
I'm not entirely sure you are real.



Re: Simplify NET_LOCK() variations

2020-04-12 Thread Mark Kettenis
> From: "Theo de Raadt" 
> Date: Sun, 12 Apr 2020 10:28:59 -0600
> 
> > + if ((p->p_flag & P_SYSTEM) &&
> > + (strncmp(p->p_p->ps_comm, "softnet", 7) == 0))
> 
> Wow that is ugly.  

A better approach might be to store a pointer to the softnet task's
struct proc in a global variable and check that.  That is what we do
for the pagedaemon for example.



Re: Simplify NET_LOCK() variations

2020-04-12 Thread Theo de Raadt
> + if ((p->p_flag & P_SYSTEM) &&
> + (strncmp(p->p_p->ps_comm, "softnet", 7) == 0))

Wow that is ugly.  



Re: Remove some customization from our perl build

2020-04-12 Thread Theo de Raadt
Andrew Hewus Fresh  wrote:

> On Fri, Apr 10, 2020 at 07:34:21PM -0600, Todd C. Miller wrote:
> > On Fri, 10 Apr 2020 18:17:33 -0700, Andrew Hewus Fresh wrote:
> > 
> > > Recently it was pointed out that we don't link /usr/lib/libperl.so.* to
> > > libm the way is expected for code that also links to libperl.  That led
> > > me to go digging again into the customization we have around the perl
> > > build and getting terribly confused.  That did somewhat clear up after
> > > reading more about bsd.*.mk, but still feel like some of this mess was
> > > to make the vax work, but I couldn't actually figure it out from the cvs
> > > logs why it exists.
> > 
> > The way OpenBSD builds is that the libraries are built and installed
> > first, then the binaries that link against those binaries get built
> > and linked against the new libs.  This guarantees that we don't
> > link new binaries against the old libs.
> 
> 
> I was so confused, had to rebuild with and without and compare logs, and
> it didn't seem like this was happening for libperl.  Then I figured out
> it stopped happening for libperl a while back.  So, it does seem like
> we shouldn't need all this code anymore.
> 
> 
> http://cvsweb.openbsd.org/src/Makefile?rev=1.92=text/x-cvsweb-markup
> http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/Makefile.diff?r1=1.91=1.92=h
> 
> https://github.com/openbsd/src/commit/7c8333e2365b13cb84a047094edcf2469959cffa

That's right, early-entry into this directory was stopped years and years ago.



Re: Remove some customization from our perl build

2020-04-12 Thread Andrew Hewus Fresh
On Fri, Apr 10, 2020 at 07:34:21PM -0600, Todd C. Miller wrote:
> On Fri, 10 Apr 2020 18:17:33 -0700, Andrew Hewus Fresh wrote:
> 
> > Recently it was pointed out that we don't link /usr/lib/libperl.so.* to
> > libm the way is expected for code that also links to libperl.  That led
> > me to go digging again into the customization we have around the perl
> > build and getting terribly confused.  That did somewhat clear up after
> > reading more about bsd.*.mk, but still feel like some of this mess was
> > to make the vax work, but I couldn't actually figure it out from the cvs
> > logs why it exists.
> 
> The way OpenBSD builds is that the libraries are built and installed
> first, then the binaries that link against those binaries get built
> and linked against the new libs.  This guarantees that we don't
> link new binaries against the old libs.


I was so confused, had to rebuild with and without and compare logs, and
it didn't seem like this was happening for libperl.  Then I figured out
it stopped happening for libperl a while back.  So, it does seem like
we shouldn't need all this code anymore.


http://cvsweb.openbsd.org/src/Makefile?rev=1.92=text/x-cvsweb-markup
http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/Makefile.diff?r1=1.91=1.92=h

https://github.com/openbsd/src/commit/7c8333e2365b13cb84a047094edcf2469959cffa


l8rZ,
-- 
andrew - http://afresh1.com

Instructions are just another man's opinion of how to do something. 
  -- Weldboy #DPWisdom



Raspberry Pi4 Ethernet support

2020-04-12 Thread Mark Kettenis
The diff below adds support for the Ethernet interface on the
Raspberry Pi4.  The new driver is called bse(4) for Broadcom SoC
Ethernet ;).  It is a port of the NetBSD driver (where the driver is
called genet(4), but over here we prefer short names for our network
drivers).  There have been many changes to adapt the driver to our
codebase, in particular the ring accounting framework.  Only the ACPI
attachment is provided for now.  An FDT attachment will follow later.
Seems I can sustain ~700 Mbit/s with this diff.

This is the complete diff including the acpi_getprop() and brgphy(4)
diffs I posted earlier.  It is my intention to commit those
separately.

ok?


Index: arch/arm64/conf/GENERIC
===
RCS file: /cvs/src/sys/arch/arm64/conf/GENERIC,v
retrieving revision 1.149
diff -u -p -r1.149 GENERIC
--- arch/arm64/conf/GENERIC 3 Apr 2020 16:29:17 -   1.149
+++ arch/arm64/conf/GENERIC 12 Apr 2020 15:38:42 -
@@ -138,12 +138,13 @@ ssdfb*at spi?
 wsdisplay* at ssdfb?
 imxtmu*at fdt?
 
-# Raspberry Pi 3
+# Raspberry Pi 3/4
 bcmaux*at fdt?
 bcmintc*   at fdt?
 bcmdog*at fdt?
 bcmrng*at fdt?
 bcmtemp*   at fdt?
+bse*   at acpi?
 dwctwo*at fdt?
 usb*   at dwctwo?
 
@@ -383,6 +384,7 @@ bwfm*   at uhub?# Broadcom 
FullMAC
 
 amphy* at mii? # AMD 79C873 PHYs
 atphy* at mii? # Attansic F1 PHYs
+brgphy*at mii? # Broadcom Gigabit PHYs
 eephy* at mii? # Marvell 88E1000 series PHY
 rgephy*at mii? # Realtek 8169S/8110S PHY
 rlphy* at mii? # Realtek 8139 internal PHYs
Index: conf/files
===
RCS file: /cvs/src/sys/conf/files,v
retrieving revision 1.685
diff -u -p -r1.685 files
--- conf/files  2 Mar 2020 10:37:22 -   1.685
+++ conf/files  12 Apr 2020 15:38:43 -
@@ -306,6 +306,10 @@ file   dev/ic/gem.cgem
 device ti: ether, ifnet, ifmedia, mii, firmload
 file   dev/ic/ti.c ti
 
+# Broadcom BCM7XXX Ethernet controller
+device bse: ether, ifnet, ifmedia, mii
+file   dev/ic/bcmgenet.c   bse
+
 # 8250/16[45]50-based "com" ports
 device com: tty
 file   dev/ic/com.ccom & (com | com_cardbus | com_gsc |
Index: dev/acpi/acpi.c
===
RCS file: /cvs/src/sys/dev/acpi/acpi.c,v
retrieving revision 1.381
diff -u -p -r1.381 acpi.c
--- dev/acpi/acpi.c 12 Apr 2020 09:21:19 -  1.381
+++ dev/acpi/acpi.c 12 Apr 2020 15:38:43 -
@@ -2961,6 +2961,57 @@ acpi_foundsony(struct aml_node *node, vo
 
 /* Support for _DSD Device Properties. */
 
+int
+acpi_getprop(struct aml_node *node, const char *prop, void *buf, int buflen)
+{
+   struct aml_value dsd;
+   int i;
+
+   /* daffd814-6eba-4d8c-8a91-bc9bbf4aa301 */
+   static uint8_t prop_guid[] = {
+   0x14, 0xd8, 0xff, 0xda, 0xba, 0x6e, 0x8c, 0x4d,
+   0x8a, 0x91, 0xbc, 0x9b, 0xbf, 0x4a, 0xa3, 0x01,
+   };
+
+   if (aml_evalname(acpi_softc, node, "_DSD", 0, NULL, ))
+   return -1;
+
+   if (dsd.type != AML_OBJTYPE_PACKAGE || dsd.length != 2 ||
+   dsd.v_package[0]->type != AML_OBJTYPE_BUFFER ||
+   dsd.v_package[1]->type != AML_OBJTYPE_PACKAGE)
+   return -1;
+
+   /* Check UUID. */
+   if (dsd.v_package[0]->length != sizeof(prop_guid) ||
+   memcmp(dsd.v_package[0]->v_buffer, prop_guid,
+   sizeof(prop_guid)) != 0)
+   return -1;
+
+   /* Check properties. */
+   for (i = 0; i < dsd.v_package[1]->length; i++) {
+   struct aml_value *res = dsd.v_package[1]->v_package[i];
+   int len;
+
+   if (res->type != AML_OBJTYPE_PACKAGE || res->length != 2 ||
+   res->v_package[0]->type != AML_OBJTYPE_STRING)
+   continue;
+
+   len = res->v_package[1]->length;
+   switch (res->v_package[1]->type) {
+   case AML_OBJTYPE_BUFFER:
+   memcpy(buf, res->v_package[1]->v_buffer,
+   min(len, buflen));
+   return len;
+   case AML_OBJTYPE_STRING:
+   memcpy(buf, res->v_package[1]->v_string,
+   min(len, buflen));
+   return len;
+   }
+   }
+
+   return -1;
+}
+
 uint32_t
 acpi_getpropint(struct aml_node *node, const char *prop, uint32_t defval)
 {
@@ -2999,7 +3050,7 @@ acpi_getpropint(struct aml_node *node, c
if (strcmp(res->v_package[0]->v_string, prop) == 0)
return 

Implement acpi_getprop()

2020-04-12 Thread Mark Kettenis
Implement the equivalent of OF_getprop() for device-tree-like
properties in ACPI land.  Needed for the upcomong Raspberry Pi4
network interface driver.

ok?


Index: dev/acpi/acpi.c
===
RCS file: /cvs/src/sys/dev/acpi/acpi.c,v
retrieving revision 1.381
diff -u -p -r1.381 acpi.c
--- dev/acpi/acpi.c 12 Apr 2020 09:21:19 -  1.381
+++ dev/acpi/acpi.c 12 Apr 2020 14:57:21 -
@@ -2961,6 +2961,57 @@ acpi_foundsony(struct aml_node *node, vo
 
 /* Support for _DSD Device Properties. */
 
+int
+acpi_getprop(struct aml_node *node, const char *prop, void *buf, int buflen)
+{
+   struct aml_value dsd;
+   int i;
+
+   /* daffd814-6eba-4d8c-8a91-bc9bbf4aa301 */
+   static uint8_t prop_guid[] = {
+   0x14, 0xd8, 0xff, 0xda, 0xba, 0x6e, 0x8c, 0x4d,
+   0x8a, 0x91, 0xbc, 0x9b, 0xbf, 0x4a, 0xa3, 0x01,
+   };
+
+   if (aml_evalname(acpi_softc, node, "_DSD", 0, NULL, ))
+   return -1;
+
+   if (dsd.type != AML_OBJTYPE_PACKAGE || dsd.length != 2 ||
+   dsd.v_package[0]->type != AML_OBJTYPE_BUFFER ||
+   dsd.v_package[1]->type != AML_OBJTYPE_PACKAGE)
+   return -1;
+
+   /* Check UUID. */
+   if (dsd.v_package[0]->length != sizeof(prop_guid) ||
+   memcmp(dsd.v_package[0]->v_buffer, prop_guid,
+   sizeof(prop_guid)) != 0)
+   return -1;
+
+   /* Check properties. */
+   for (i = 0; i < dsd.v_package[1]->length; i++) {
+   struct aml_value *res = dsd.v_package[1]->v_package[i];
+   int len;
+
+   if (res->type != AML_OBJTYPE_PACKAGE || res->length != 2 ||
+   res->v_package[0]->type != AML_OBJTYPE_STRING)
+   continue;
+
+   len = res->v_package[1]->length;
+   switch (res->v_package[1]->type) {
+   case AML_OBJTYPE_BUFFER:
+   memcpy(buf, res->v_package[1]->v_buffer,
+   min(len, buflen));
+   return len;
+   case AML_OBJTYPE_STRING:
+   memcpy(buf, res->v_package[1]->v_string,
+   min(len, buflen));
+   return len;
+   }
+   }
+
+   return -1;
+}
+
 uint32_t
 acpi_getpropint(struct aml_node *node, const char *prop, uint32_t defval)
 {
@@ -2999,7 +3050,7 @@ acpi_getpropint(struct aml_node *node, c
if (strcmp(res->v_package[0]->v_string, prop) == 0)
return res->v_package[1]->v_integer;
}
-   
+
return defval;
 }
 
Index: dev/acpi/acpivar.h
===
RCS file: /cvs/src/sys/dev/acpi/acpivar.h,v
retrieving revision 1.106
diff -u -p -r1.106 acpivar.h
--- dev/acpi/acpivar.h  12 Apr 2020 09:21:19 -  1.106
+++ dev/acpi/acpivar.h  12 Apr 2020 14:57:21 -
@@ -372,6 +372,7 @@ int acpi_matchhids(struct acpi_attach_ar
 intacpi_parsehid(struct aml_node *, void *, char *, char *, size_t);
 int64_tacpi_getsta(struct acpi_softc *sc, struct aml_node *);
 
+intacpi_getprop(struct aml_node *, const char *, void *, int);
 uint32_t acpi_getpropint(struct aml_node *, const char *, uint32_t);
 
 intacpi_record_event(struct acpi_softc *, u_int);



Re: Simplify NET_LOCK() variations

2020-04-12 Thread Mark Kettenis
> Date: Sun, 12 Apr 2020 14:38:55 +
> From: Visa Hankala 
> 
> On Sun, Apr 12, 2020 at 03:26:14PM +0200, Martin Pieuchot wrote:
> > The existing variations of the NET_LOCK() macros are confusing.  We're
> > now all aware of this fact.  So let's keep them simple to prevent future
> > mistakes :)
> > 
> > The diff below reduces the current set of methods to the following:
> > 
> > NET_LOCK()/NET_UNLOCK()
> > NET_ASSERT_LOCKED()
> > NET_ASSERT_UNLOCKED()
> > 
> > On top of those, two extra methods are provided for a *very specific*
> > purpose:
> > 
> > NET_RLOCK_IN_IOCTL()
> > NET_RUNLOCK_IN_IOCTL()
> > 
> > "IN_IOCTL()" mean they should only be used in ioctl(2) context ;)  The
> > purpose is to keep previous behavior where read-only ioctl(2) dot not
> > wait for packet processing to finish. 
> > 
> > To achieve this NET_LOCK() and NET_UNLOCK() now contain some magic to
> > ensure the only packet processing thread grabbing a read-lock is the
> > softnet thread.  In other words, one doesn't need to be aware of all
> > this magic, just imagine that the Network Stack is big-locked and use
> > NET_LOCK()/NET_UNLOCK() everywhere.
> 
> I think it is a potential source of error if NET_LOCK() is sensitive
> to context. To understand what this code does, you have to know which
> thread calls it.
> 
> I would prefer to have a separate NET_RLOCK variant for explicit use
> by softnet threads, say NET_RLOCK_IN_SOFTNET() or NET_RLOCK_SOFTNET(),
> which can assert that the calling context is correct and read-lock
> netlock.

I think what visa@ says here makes sense.



Re: Simplify NET_LOCK() variations

2020-04-12 Thread Visa Hankala
On Sun, Apr 12, 2020 at 03:26:14PM +0200, Martin Pieuchot wrote:
> The existing variations of the NET_LOCK() macros are confusing.  We're
> now all aware of this fact.  So let's keep them simple to prevent future
> mistakes :)
> 
> The diff below reduces the current set of methods to the following:
> 
>   NET_LOCK()/NET_UNLOCK()
>   NET_ASSERT_LOCKED()
>   NET_ASSERT_UNLOCKED()
> 
> On top of those, two extra methods are provided for a *very specific*
> purpose:
> 
>   NET_RLOCK_IN_IOCTL()
>   NET_RUNLOCK_IN_IOCTL()
> 
> "IN_IOCTL()" mean they should only be used in ioctl(2) context ;)  The
> purpose is to keep previous behavior where read-only ioctl(2) dot not
> wait for packet processing to finish. 
> 
> To achieve this NET_LOCK() and NET_UNLOCK() now contain some magic to
> ensure the only packet processing thread grabbing a read-lock is the
> softnet thread.  In other words, one doesn't need to be aware of all
> this magic, just imagine that the Network Stack is big-locked and use
> NET_LOCK()/NET_UNLOCK() everywhere.

I think it is a potential source of error if NET_LOCK() is sensitive
to context. To understand what this code does, you have to know which
thread calls it.

I would prefer to have a separate NET_RLOCK variant for explicit use
by softnet threads, say NET_RLOCK_IN_SOFTNET() or NET_RLOCK_SOFTNET(),
which can assert that the calling context is correct and read-lock
netlock.



Re: Simplify NET_LOCK() variations

2020-04-12 Thread Vitaliy Makkoveev



> On 12 Apr 2020, at 16:26, Martin Pieuchot  wrote:
> 
> The existing variations of the NET_LOCK() macros are confusing.  We're
> now all aware of this fact.  So let's keep them simple to prevent future
> mistakes :)
> 
> The diff below reduces the current set of methods to the following:
> 
>   NET_LOCK()/NET_UNLOCK()
>   NET_ASSERT_LOCKED()
>   NET_ASSERT_UNLOCKED()
> 
> On top of those, two extra methods are provided for a *very specific*
> purpose:
> 
>   NET_RLOCK_IN_IOCTL()
>   NET_RUNLOCK_IN_IOCTL()
> 
> "IN_IOCTL()" mean they should only be used in ioctl(2) context ;)  The
> purpose is to keep previous behavior where read-only ioctl(2) dot not
> wait for packet processing to finish. 
> 
> To achieve this NET_LOCK() and NET_UNLOCK() now contain some magic to
> ensure the only packet processing thread grabbing a read-lock is the
> softnet thread.  In other words, one doesn't need to be aware of all
> this magic, just imagine that the Network Stack is big-locked and use
> NET_LOCK()/NET_UNLOCK() everywhere.
> 
> If you think this is an improvement.  I'd like to know if there is a
> better way to implement the magic in NET_LOCK().  String comparison is
> not great ;)

What about new special P_ flag? Like P_SOFTDEP used by syncerproc.

> 
> Comments?  What do you think about this?  I'm interested to hear all of
> you, 'case I'd like to know how does everyone perceive this complexity
> related to locks.
> 
> Thanks for reading.
> 
> PS: macros have been converted to functions to avoid pulling more
> headers in , the magic requires .
> 
> Index: net/if.c
> ===
> RCS file: /cvs/src/sys/net/if.c,v
> retrieving revision 1.603
> diff -u -p -r1.603 if.c
> --- net/if.c  12 Apr 2020 07:04:03 -  1.603
> +++ net/if.c  12 Apr 2020 12:58:10 -
> @@ -250,6 +250,60 @@ struct task if_input_task_locked = TASK_
> struct rwlock netlock = RWLOCK_INITIALIZER("netlock");
> 
> /*
> + * Network stack data structures are, unless stated otherwise, protected
> + * by the NET_LOCK().  It's a single non-recursive lock for the whole
> + * subsystem.
> + */
> +void
> +NET_LOCK(void)
> +{
> + struct proc *p = curproc;
> +
> + /*
> +  * The "softnet" thread should be the only thread processing
> +  * packets without holding an exclusive lock.  This is done
> +  * to allow read-only ioctl(2) to not block.
> +  */
> + if ((p->p_flag & P_SYSTEM) &&
> + (strncmp(p->p_p->ps_comm, "softnet", 7) == 0))
> + rw_enter_read();
> + else
> + rw_enter_write();
> +}
> +
> +void
> +NET_UNLOCK(void)
> +{
> + struct proc *p = curproc;
> +
> + if ((p->p_flag & P_SYSTEM) &&
> + (strncmp(p->p_p->ps_comm, "softnet", 7) == 0))
> + rw_exit_read();
> + else
> + rw_exit_write();
> +}
> +
> +/*
> + * Reader version of NET_LOCK() to be used in ioctl(2) path only.
> + *
> + * Can be grabbed instead of the exclusive version when no field
> + * protected by the NET_LOCK() is modified by the ioctl(2).
> + */
> +void
> +NET_RLOCK_IN_IOCTL(void)
> +{
> + KASSERT((curproc->p_flag & P_SYSTEM) == 0);
> + rw_enter_read();
> +}
> +
> +void
> +NET_RUNLOCK_IN_IOCTL(void)
> +{
> + KASSERT((curproc->p_flag & P_SYSTEM) == 0);
> + rw_exit_read();
> +}
> +
> +/*
>  * Network interface utility routines.
>  */
> void
> @@ -936,7 +990,7 @@ if_input_process(struct ifnet *ifp, stru
>*
>* Since we have a NET_LOCK() we also use it to serialize access
>* to PF globals, pipex globals, unicast and multicast addresses
> -  * lists.
> +  * lists and the socket layer.
>*/
>   NET_LOCK();
>   while ((m = ml_dequeue(ml)) != NULL)
> @@ -2338,27 +2392,27 @@ ifioctl_get(u_long cmd, caddr_t data)
> 
>   switch(cmd) {
>   case SIOCGIFCONF:
> - NET_RLOCK();
> + NET_RLOCK_IN_IOCTL();
>   error = ifconf(data);
> - NET_RUNLOCK();
> + NET_RUNLOCK_IN_IOCTL();
>   return (error);
>   case SIOCIFGCLONERS:
>   error = if_clone_list((struct if_clonereq *)data);
>   return (error);
>   case SIOCGIFGMEMB:
> - NET_RLOCK();
> + NET_RLOCK_IN_IOCTL();
>   error = if_getgroupmembers(data);
> - NET_RUNLOCK();
> + NET_RUNLOCK_IN_IOCTL();
>   return (error);
>   case SIOCGIFGATTR:
> - NET_RLOCK();
> + NET_RLOCK_IN_IOCTL();
>   error = if_getgroupattribs(data);
> - NET_RUNLOCK();
> + NET_RUNLOCK_IN_IOCTL();
>   return (error);
>   case SIOCGIFGLIST:
> - NET_RLOCK();
> + NET_RLOCK_IN_IOCTL();
>   error = if_getgrouplist(data);
> - NET_RUNLOCK();
> + NET_RUNLOCK_IN_IOCTL();
>   return (error);
>   }
> 
> @@ -2366,7 +2420,7 

Simplify NET_LOCK() variations

2020-04-12 Thread Martin Pieuchot
The existing variations of the NET_LOCK() macros are confusing.  We're
now all aware of this fact.  So let's keep them simple to prevent future
mistakes :)

The diff below reduces the current set of methods to the following:

NET_LOCK()/NET_UNLOCK()
NET_ASSERT_LOCKED()
NET_ASSERT_UNLOCKED()

On top of those, two extra methods are provided for a *very specific*
purpose:

NET_RLOCK_IN_IOCTL()
NET_RUNLOCK_IN_IOCTL()

"IN_IOCTL()" mean they should only be used in ioctl(2) context ;)  The
purpose is to keep previous behavior where read-only ioctl(2) dot not
wait for packet processing to finish. 

To achieve this NET_LOCK() and NET_UNLOCK() now contain some magic to
ensure the only packet processing thread grabbing a read-lock is the
softnet thread.  In other words, one doesn't need to be aware of all
this magic, just imagine that the Network Stack is big-locked and use
NET_LOCK()/NET_UNLOCK() everywhere.

If you think this is an improvement.  I'd like to know if there is a
better way to implement the magic in NET_LOCK().  String comparison is
not great ;)

Comments?  What do you think about this?  I'm interested to hear all of
you, 'case I'd like to know how does everyone perceive this complexity
related to locks.

Thanks for reading.

PS: macros have been converted to functions to avoid pulling more
headers in , the magic requires .

Index: net/if.c
===
RCS file: /cvs/src/sys/net/if.c,v
retrieving revision 1.603
diff -u -p -r1.603 if.c
--- net/if.c12 Apr 2020 07:04:03 -  1.603
+++ net/if.c12 Apr 2020 12:58:10 -
@@ -250,6 +250,60 @@ struct task if_input_task_locked = TASK_
 struct rwlock netlock = RWLOCK_INITIALIZER("netlock");
 
 /*
+ * Network stack data structures are, unless stated otherwise, protected
+ * by the NET_LOCK().  It's a single non-recursive lock for the whole
+ * subsystem.
+ */
+void
+NET_LOCK(void)
+{
+   struct proc *p = curproc;
+
+   /*
+* The "softnet" thread should be the only thread processing
+* packets without holding an exclusive lock.  This is done
+* to allow read-only ioctl(2) to not block.
+*/
+   if ((p->p_flag & P_SYSTEM) &&
+   (strncmp(p->p_p->ps_comm, "softnet", 7) == 0))
+   rw_enter_read();
+   else
+   rw_enter_write();
+}
+
+void
+NET_UNLOCK(void)
+{
+   struct proc *p = curproc;
+
+   if ((p->p_flag & P_SYSTEM) &&
+   (strncmp(p->p_p->ps_comm, "softnet", 7) == 0))
+   rw_exit_read();
+   else
+   rw_exit_write();
+}
+
+/*
+ * Reader version of NET_LOCK() to be used in ioctl(2) path only.
+ *
+ * Can be grabbed instead of the exclusive version when no field
+ * protected by the NET_LOCK() is modified by the ioctl(2).
+ */
+void
+NET_RLOCK_IN_IOCTL(void)
+{
+   KASSERT((curproc->p_flag & P_SYSTEM) == 0);
+   rw_enter_read();
+}
+
+void
+NET_RUNLOCK_IN_IOCTL(void)
+{
+   KASSERT((curproc->p_flag & P_SYSTEM) == 0);
+   rw_exit_read();
+}
+
+/*
  * Network interface utility routines.
  */
 void
@@ -936,7 +990,7 @@ if_input_process(struct ifnet *ifp, stru
 *
 * Since we have a NET_LOCK() we also use it to serialize access
 * to PF globals, pipex globals, unicast and multicast addresses
-* lists.
+* lists and the socket layer.
 */
NET_LOCK();
while ((m = ml_dequeue(ml)) != NULL)
@@ -2338,27 +2392,27 @@ ifioctl_get(u_long cmd, caddr_t data)
 
switch(cmd) {
case SIOCGIFCONF:
-   NET_RLOCK();
+   NET_RLOCK_IN_IOCTL();
error = ifconf(data);
-   NET_RUNLOCK();
+   NET_RUNLOCK_IN_IOCTL();
return (error);
case SIOCIFGCLONERS:
error = if_clone_list((struct if_clonereq *)data);
return (error);
case SIOCGIFGMEMB:
-   NET_RLOCK();
+   NET_RLOCK_IN_IOCTL();
error = if_getgroupmembers(data);
-   NET_RUNLOCK();
+   NET_RUNLOCK_IN_IOCTL();
return (error);
case SIOCGIFGATTR:
-   NET_RLOCK();
+   NET_RLOCK_IN_IOCTL();
error = if_getgroupattribs(data);
-   NET_RUNLOCK();
+   NET_RUNLOCK_IN_IOCTL();
return (error);
case SIOCGIFGLIST:
-   NET_RLOCK();
+   NET_RLOCK_IN_IOCTL();
error = if_getgrouplist(data);
-   NET_RUNLOCK();
+   NET_RUNLOCK_IN_IOCTL();
return (error);
}
 
@@ -2366,7 +2420,7 @@ ifioctl_get(u_long cmd, caddr_t data)
if (ifp == NULL)
return (ENXIO);
 
-   NET_RLOCK();
+   NET_RLOCK_IN_IOCTL();
 
switch(cmd) {
case SIOCGIFFLAGS:
@@ -2434,7 +2488,7 @@ ifioctl_get(u_long cmd, caddr_t data)

Re: switch powerpc to MI mplock

2020-04-12 Thread Otto Moerbeek
On Fri, Apr 10, 2020 at 09:31:24AM +0200, Martin Pieuchot wrote:

> In order to reduce the differences with other architecture and to be able
> to use WITNESS on powerpc I'm proposing the diff below that makes use of
> the MI mp (ticket) lock implementation for powerpc.
> 
> This has been tested by Peter J. Philipp but I'd like to have more tests
> before proceeding.
> 
> As explained previously the pmap code, which is using a recursive
> spinlock to protect the hash, still uses the old lock implementation with
> this diff.
> 
> Please fire your MP macppc and report back :o)

Did a build on a dual cpu G4 with this diff without incident.

-Otto

> 
> Index: arch/powerpc/include/mplock.h
> ===
> RCS file: /cvs/src/sys/arch/powerpc/include/mplock.h,v
> retrieving revision 1.3
> diff -u -p -r1.3 mplock.h
> --- arch/powerpc/include/mplock.h 4 Dec 2017 09:51:03 -   1.3
> +++ arch/powerpc/include/mplock.h 9 Apr 2020 16:21:55 -
> @@ -27,25 +27,27 @@
>  #ifndef _POWERPC_MPLOCK_H_
>  #define _POWERPC_MPLOCK_H_
>  
> +#define __USE_MI_MPLOCK
> +
>  /*
>   * Really simple spinlock implementation with recursive capabilities.
>   * Correctness is paramount, no fancyness allowed.
>   */
>  
> -struct __mp_lock {
> +struct __ppc_lock {
>   volatile struct cpu_info *mpl_cpu;
>   volatile long   mpl_count;
>  };
>  
>  #ifndef _LOCORE
>  
> -void __mp_lock_init(struct __mp_lock *);
> -void __mp_lock(struct __mp_lock *);
> -void __mp_unlock(struct __mp_lock *);
> -int __mp_release_all(struct __mp_lock *);
> -int __mp_release_all_but_one(struct __mp_lock *);
> -void __mp_acquire_count(struct __mp_lock *, int);
> -int __mp_lock_held(struct __mp_lock *, struct cpu_info *);
> +void __ppc_lock_init(struct __ppc_lock *);
> +void __ppc_lock(struct __ppc_lock *);
> +void __ppc_unlock(struct __ppc_lock *);
> +int __ppc_release_all(struct __ppc_lock *);
> +int __ppc_release_all_but_one(struct __ppc_lock *);
> +void __ppc_acquire_count(struct __ppc_lock *, int);
> +int __ppc_lock_held(struct __ppc_lock *, struct cpu_info *);
>  
>  #endif
>  
> Index: arch/powerpc/powerpc/lock_machdep.c
> ===
> RCS file: /cvs/src/sys/arch/powerpc/powerpc/lock_machdep.c,v
> retrieving revision 1.8
> diff -u -p -r1.8 lock_machdep.c
> --- arch/powerpc/powerpc/lock_machdep.c   5 Mar 2020 09:28:31 -   
> 1.8
> +++ arch/powerpc/powerpc/lock_machdep.c   9 Apr 2020 16:21:01 -
> @@ -27,7 +27,7 @@
>  #include 
>  
>  void
> -__mp_lock_init(struct __mp_lock *lock)
> +__ppc_lock_init(struct __ppc_lock *lock)
>  {
>   lock->mpl_cpu = NULL;
>   lock->mpl_count = 0;
> @@ -43,7 +43,7 @@ extern int __mp_lock_spinout;
>  #endif
>  
>  static __inline void
> -__mp_lock_spin(struct __mp_lock *mpl)
> +__ppc_lock_spin(struct __ppc_lock *mpl)
>  {
>  #ifndef MP_LOCKDEBUG
>   while (mpl->mpl_count != 0)
> @@ -55,14 +55,14 @@ __mp_lock_spin(struct __mp_lock *mpl)
>   CPU_BUSY_CYCLE();
>  
>   if (nticks == 0) {
> - db_printf("__mp_lock(%p): lock spun out\n", mpl);
> + db_printf("__ppc_lock(%p): lock spun out\n", mpl);
>   db_enter();
>   }
>  #endif
>  }
>  
>  void
> -__mp_lock(struct __mp_lock *mpl)
> +__ppc_lock(struct __ppc_lock *mpl)
>  {
>   /*
>* Please notice that mpl_count gets incremented twice for the
> @@ -92,18 +92,18 @@ __mp_lock(struct __mp_lock *mpl)
>   }
>   ppc_intr_enable(s);
>  
> - __mp_lock_spin(mpl);
> + __ppc_lock_spin(mpl);
>   }
>  }
>  
>  void
> -__mp_unlock(struct __mp_lock *mpl)
> +__ppc_unlock(struct __ppc_lock *mpl)
>  {
>   int s;
>  
>  #ifdef MP_LOCKDEBUG
>   if (mpl->mpl_cpu != curcpu()) {
> - db_printf("__mp_unlock(%p): not held lock\n", mpl);
> + db_printf("__ppc_unlock(%p): not held lock\n", mpl);
>   db_enter();
>   }
>  #endif
> @@ -118,14 +118,14 @@ __mp_unlock(struct __mp_lock *mpl)
>  }
>  
>  int
> -__mp_release_all(struct __mp_lock *mpl)
> +__ppc_release_all(struct __ppc_lock *mpl)
>  {
>   int rv = mpl->mpl_count - 1;
>   int s;
>  
>  #ifdef MP_LOCKDEBUG
>   if (mpl->mpl_cpu != curcpu()) {
> - db_printf("__mp_release_all(%p): not held lock\n", mpl);
> + db_printf("__ppc_release_all(%p): not held lock\n", mpl);
>   db_enter();
>   }
>  #endif
> @@ -140,13 +140,13 @@ __mp_release_all(struct __mp_lock *mpl)
>  }
>  
>  int
> -__mp_release_all_but_one(struct __mp_lock *mpl)
> +__ppc_release_all_but_one(struct __ppc_lock *mpl)
>  {
>   int rv = mpl->mpl_count - 2;
>  
>  #ifdef MP_LOCKDEBUG
>   if (mpl->mpl_cpu != curcpu()) {
> - db_printf("__mp_release_all_but_one(%p): not held lock\n", mpl);
> + db_printf("__ppc_release_all_but_one(%p): not held lock\n", 
> mpl);
>   

fix pppac(4) without pipex

2020-04-12 Thread YASUOKA Masahiko
Hi,

The diff followings fixes panics when using pppac(4) with "pipex no".

Index: sys/net/if_pppx.c
===
RCS file: /cvs/src/sys/net/if_pppx.c,v
retrieving revision 1.83
diff -u -p -r1.83 if_pppx.c
--- sys/net/if_pppx.c   10 Apr 2020 07:36:52 -  1.83
+++ sys/net/if_pppx.c   12 Apr 2020 06:12:35 -
@@ -344,7 +344,7 @@ pppxwrite(dev_t dev, struct uio *uio, in
if (m == NULL)
return (ENOBUFS);
mlen = MHLEN;
-   if (uio->uio_resid >= MINCLSIZE) {
+   if (uio->uio_resid > MHLEN) {
MCLGET(m, M_DONTWAIT);
if (!(m->m_flags & M_EXT)) {
m_free(m);
@@ -1368,7 +1368,7 @@ pppacwrite(dev_t dev, struct uio *uio, i
if (m == NULL)
return (ENOMEM);
 
-   if (uio->uio_resid > MINCLSIZE) {
+   if (uio->uio_resid > MHLEN) {
m_clget(m, M_WAITOK, uio->uio_resid);
if (!ISSET(m->m_flags, M_EXT)) {
m_free(m);