Re: Make pipex more common for pppac and pppx

2020-08-19 Thread YASUOKA Masahiko
Hi,

Thank you for your comments.

On Mon, 17 Aug 2020 00:15:08 +0300
Vitaliy Makkoveev  wrote:
> I like your idea to kill `pipex_iface_context'. I had trying to keep it
> by myself and this was wrong way. Could you rework your diff to be
> against the recent sources?

I'm sorry the diff was for the old version.

>> @@ -1122,8 +1051,11 @@ pppacopen(dev_t dev, int flags, int mode, struct proc 
>> *p)
>>  #if NBPFILTER > 0
>>  bpfattach(>if_bpf, ifp, DLT_LOOP, sizeof(uint32_t));
>>  #endif
>> -
>> -pipex_iface_init(>sc_pipex_iface, ifp->if_index);
>> +/* virtual pipex_session entry for multicast */
>> +session = pool_get(_session_pool, PR_WAITOK | PR_ZERO);
>> +session->is_multicast = 1;
>> +session->ifindex = ifp->if_index;
>> +sc->sc_multicast_session = session;
>>  
> Interface index is not required for multicast session, because it's
> never used. Also I like to alloc `sc_multicast_session' before
> if_attach().

The diff was to use `ifindex' to select all sessions associated the
same pppac(4).  But the latest diff uses `ownersc' instead for the
same purpose.  Also the allocation was moved to earlier part of the
function.

>> @@ -1382,7 +1340,10 @@ pppacclose(dev_t dev, int flags, int mode, struct 
>> proc *p)
>>  klist_invalidate(>sc_wsel.si_note);
>>  splx(s);
>>  
>> -pipex_iface_fini(>sc_pipex_iface);
>> +pool_put(_session_pool, sc->sc_multicast_session);
>> +NET_LOCK();
>> +pipex_destroy_all_sessions(sc);
>> +NET_UNLOCK();
>>  
>>  if_detach(ifp);
> 
> The recent sources has pppac(4) with unlocked start routine. I like you
> detach `ifp' before destroy `sc_multicast_session'.

The lines were moved after if_detach().

I'll test this more on this weekend, then I'll ask ok for this.

Index: sys/net/if_pppx.c
===
RCS file: /cvs/src/sys/net/if_pppx.c,v
retrieving revision 1.101
diff -u -p -r1.101 if_pppx.c
--- sys/net/if_pppx.c   14 Aug 2020 11:05:38 -  1.101
+++ sys/net/if_pppx.c   20 Aug 2020 05:19:55 -
@@ -163,7 +163,6 @@ struct pppx_if {
struct ifnetpxi_if;
struct pppx_dev *pxi_dev;   /* [I] */
struct pipex_session*pxi_session;   /* [I] */
-   struct pipex_iface_context  pxi_ifcontext;  /* [N] */
 };
 
 static inline int
@@ -181,12 +180,6 @@ intpppx_add_session(struct pppx_dev *,
struct pipex_session_req *);
 intpppx_del_session(struct pppx_dev *,
struct pipex_session_close_req *);
-intpppx_config_session(struct pppx_dev *,
-   struct pipex_session_config_req *);
-intpppx_get_stat(struct pppx_dev *,
-   struct pipex_session_stat_req *);
-intpppx_get_closed(struct pppx_dev *,
-   struct pipex_session_list_req *);
 intpppx_set_session_descr(struct pppx_dev *,
struct pipex_session_descr_req *);
 
@@ -424,17 +417,6 @@ pppxioctl(dev_t dev, u_long cmd, caddr_t
 
NET_LOCK();
switch (cmd) {
-   case PIPEXSMODE:
-   /*
-* npppd always enables on open, and only disables before
-* closing. we cheat and let open and close do that, so lie
-* to npppd.
-*/
-   break;
-   case PIPEXGMODE:
-   *(int *)addr = 1;
-   break;
-
case PIPEXASESSION:
error = pppx_add_session(pxd,
(struct pipex_session_req *)addr);
@@ -445,21 +427,6 @@ pppxioctl(dev_t dev, u_long cmd, caddr_t
(struct pipex_session_close_req *)addr);
break;
 
-   case PIPEXCSESSION:
-   error = pppx_config_session(pxd,
-   (struct pipex_session_config_req *)addr);
-   break;
-
-   case PIPEXGSTAT:
-   error = pppx_get_stat(pxd,
-   (struct pipex_session_stat_req *)addr);
-   break;
-
-   case PIPEXGCLOSED:
-   error = pppx_get_closed(pxd,
-   (struct pipex_session_list_req *)addr);
-   break;
-
case PIPEXSIFDESCR:
error = pppx_set_session_descr(pxd,
(struct pipex_session_descr_req *)addr);
@@ -472,7 +439,7 @@ pppxioctl(dev_t dev, u_long cmd, caddr_t
break;
 
default:
-   error = ENOTTY;
+   error = pipex_ioctl(pxd, cmd, addr);
break;
}
NET_UNLOCK();
@@ -741,11 +708,7 @@ pppx_add_session(struct pppx_dev *pxd, s
if_addrhooks_run(ifp);
}
 
-   /* fake a pipex interface context */
-   pxi->pxi_ifcontext.ifindex = ifp->if_index;
-   pxi->pxi_ifcontext.pipexmode = PIPEX_ENABLED;
-
-   error = pipex_link_session(session, >pxi_ifcontext);
+   error = 

ldapd(8): fix, simplify UUID timestamp code

2020-08-19 Thread Scott Cheloha
Hi,

I was auditing the tree for odd-looking time structure usage and I
came across the UUID code in ldapd(8), uuid.c.

time_cmp() is backwards.  Or the caller is misusing it.  One or the
other.  It returns -1 if tv1 exceeds tv2 but the comments in the
caller indicate the opposite impression.  I don't think this code has
ever worked as intended.

It would be a lot easier if we just threw the code out and used random
UUIDs.  After reading over the RFC it seems to me that time-based
UUIDs are collision-prone.  Their implementation is also complicated.
Purely random UUIDs should effectively never collide and are trivial
to implement.

However, assuming we can't just use random UUIDs, here's an attempt at
improving this code:

- Use clock_gettime(2).  With nanosecond resolution we don't need
  a 'counter'.

- Reduce the scope of all the static state to uuid_create().

- Shrink the loop.  Just read the clock until it changes, then decide
  what to do re. seq_num.  This is effectively what the example code in
  RFC 4122 does.

I'm unsure what the right thing to do is if the system clock predates
the UUID epoch (Oct 15 1582).  My code just returns zero.  Maybe we
should just kill the daemon in that case?  The UUIDv1 scheme breaks
down if time is that seriously screwed up.

Is there an active ldapd(8) person?  Or at least someone with an
ldapd(8) setup who can test this?

Thoughts?

Index: uuid.c
===
RCS file: /cvs/src/usr.sbin/ldapd/uuid.c,v
retrieving revision 1.6
diff -u -p -r1.6 uuid.c
--- uuid.c  26 Apr 2018 12:42:51 -  1.6
+++ uuid.c  20 Aug 2020 01:44:00 -
@@ -63,27 +63,8 @@
 
 #include "uuid.h"
 
-static uint32_t seq_num;
-static struct timeval last_time;
-static int32_t counter;
-static char nodeaddr[6];
-
 enum { UUID_NODE_MULTICAST = 0x80 };
 
-static int
-time_cmp(struct timeval *tv1, struct timeval *tv2)
-{
-if (tv1->tv_sec > tv2->tv_sec)
-   return -1;
-if (tv1->tv_sec < tv2->tv_sec)
-   return 1;
-if (tv1->tv_usec > tv2->tv_usec)
-   return -1;
-if (tv1->tv_usec < tv2->tv_usec)
-   return 1;
-return 0;
-}
-
 static void
 get_node_addr(char *addr)
 {
@@ -138,6 +119,40 @@ get_node_addr(char *addr)
 }
 
 /*
+ * A UUID v1 timestamp:
+ *
+ * - 60 bits.
+ * - Unsigned.
+ * - Epoch at Oct 15 1582 00:00:00 UTC.
+ * - Increments every 100 nanoseconds.
+ */
+#define UUID_EPOCH_OFFSET  12219292800LL
+#define UUID_TIME_MAX  (1ULL << 60)
+#define UUID_HZ1000LL
+#define NSEC_PER_UUID_TICK 100LL
+
+static uint64_t
+get_uuid_timestamp(void)
+{
+   static const struct timespec min = { -UUID_EPOCH_OFFSET, 0 };
+   static const struct timespec max = {
+   UUID_TIME_MAX / UUID_HZ,
+   UUID_TIME_MAX % UUID_HZ * NSEC_PER_UUID_TICK
+   };
+   struct timespec utc;
+   uint64_t timestamp;
+
+   clock_gettime(CLOCK_REALTIME, );
+   if (timespeccmp(, , <))
+   return 0;
+   if (timespeccmp(, , <))
+   return UUID_TIME_MAX;
+   timestamp = (UUID_EPOCH_OFFSET + utc.tv_sec) * UUID_HZ;
+   timestamp += utc.tv_nsec / NSEC_PER_UUID_TICK;
+   return timestamp;
+}
+
+/*
  *Creates a new UUID.
  */
 
@@ -145,55 +160,32 @@ void
 uuid_create(afsUUID *uuid)
 {
 static int uuid_inited = 0;
-struct timeval tv;
-int ret, got_time;
+static uint64_t last_time;
+static uint32_t seq_num;
+static char nodeaddr[6];
 uint64_t dce_time;
 
 if (uuid_inited == 0) {
-   gettimeofday(_time, NULL);
+   last_time = get_uuid_timestamp();
seq_num = arc4random();
get_node_addr(nodeaddr);
uuid_inited = 1;
 }
 
-gettimeofday(, NULL);
-
-got_time = 0;
+while ((dce_time = get_uuid_timestamp()) == last_time)
+   continue;
 
-do {
-   ret = time_cmp(, _time);
-   if (ret < 0) {
-   /* Time went backward, just inc seq_num and be done.
-* seq_num is 6 + 8 bit field it the uuid, so let it wrap
-* around. don't let it be zero.
-*/
-   seq_num = (seq_num + 1) & 0x3fff ;
-   if (seq_num == 0)
-   seq_num++;
-   got_time = 1;
-   counter = 0;
-   last_time = tv;
-   } else if (ret > 0) {
-   /* time went forward, reset counter and be happy */
-   last_time = tv;
-   counter = 0;
-   got_time = 1;
-   } else {
-#define UUID_MAX_HZ (1) /* make this bigger fix you have larger tickrate */
-#define MULTIPLIER_100_NANO_SEC 10
-   if (++counter < UUID_MAX_HZ * MULTIPLIER_100_NANO_SEC)
-   got_time = 1;
-   }
-} while(!got_time);
+if (dce_time < last_time) {
+   /* Time went backward, just inc seq_num and be done.
+* seq_num is 6 + 8 bit field it the uuid, so let it wrap
+* around. don't let it be zero.
+*/
+   seq_num = (seq_num + 

Re: openrsync(1): add support for IPv6-only hosts

2020-08-19 Thread Florian Obser
On Wed, Aug 19, 2020 at 12:20:19AM +0200, Klemens Nanni wrote:
> On Tue, Aug 18, 2020 at 09:58:56AM +0200, Sasha Romijn wrote:
> > The current openrsync client is not able to connect to dual-stack remote 
> > hosts, when the local host does not have any IPv4 connectivity. This is 
> > because connect() fails with EADDRNOTAVAIL when trying to connect to the 
> > remote IPv4 address on an IPv6-only local host - and IPv4 seems to be 
> > attempted first. The current client does not try other remote host 
> > addresses after EADDRNOTAVAIL, and therefore never tries the remote IPv6 
> > address.
> For openrsync(1) this won't happen with `family inet6 inet4' in
> resolv.conf(5).
> 
> > The included patch allows the client to continue after EADDRNOTAVAIL, 
> > making it to try other addresses, until it reaches a working IPv6 address. 
> > If the local host is IPv6-only and the remote host IPv4-only, the 
> > connection still fails with an error produced from rsync_connect().
> Your diff reads correct to me, thanks; regular rsync(1) connects fine
> regardless of `family'.
> 
> > Perhaps a warning should be issued for the EADDRNOTAVAIL case, like it does 
> > for EHOSTUNREACH - but I thought it would be a bit much, as an IPv6-only 
> > host would then emit warnings on pretty much every single use of the 
> > openrsync client.
> Yes, I don't think a warning is warrented here - afterall it can connect
> without problems.
> 
> OK to commit anyone?
> 

Sorry for being late to the party.
Should we maybe inline inet_connect into rsync_connect and ignore all
connect(3) errors and try the host?
That's what ssh is doing in ssh_connect_direct()

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



Re: PATCH: VMD fixes for PCI Config Space and BAR Allocation [passthrough PCI support]

2020-08-19 Thread Dave Voutila


Jordan Hargrave writes:

> This is the first patch for adding PCI passthrough support to VMD.
> I am splitting up the necessary changes into smaller patches.
>
> This code fixes the pci device union for accessing PCI config space >= 0x40
> pcidump -xxx would return garbage data due to union overlap

I can confirm at least in an OpenBSD guest that pcidump is returning
something different for the virtio devices:

24,26c24,26
<   0x0040: 0001   
<   0x0050: 0010   
<   0x0060:  2000 ff4188fd 000b
---
>   0x0040:    
>   0x0050:    
>   0x0060:    
41,43c41,43
<   0x0040: 0001   
<   0x0050: 0010   
<   0x0060:  1000 ff418911 000b
---
>   0x0040:    
>   0x0050:    
>   0x0060:    
46c46
<   0x0090:   01d25048 000c
---
>   0x0090:    
58,60c58,60
<   0x0040: 0001   
<   0x0050: 0010   
<   0x0060:  d000 ff418909 000b
---
>   0x0040:    
>   0x0050:    
>   0x0060:    
63c63
<   0x0090:   019adaf5 000c
---
>   0x0090:    
75,77c75,77
<   0x0040: 0001   
<   0x0050: 0010   
<   0x0060:  2000 ff418924 000b
---
>   0x0040:    
>   0x0050:    
>   0x0060:    


I'll apply it to my other system where I keep a mix of OpenBSD and Linux
guests running.

>
> pci_add_bar now requires specifying the BAR area size to allocate
>
> Extra cookie argument added to pci_add_device
>

PCI isn't something I grok, so hopefully someone else can take a look at
this to see if it makes sense.

-Dave Voutila



Re: Fewer pool_get() in kqueue_register()

2020-08-19 Thread Visa Hankala
On Wed, Aug 19, 2020 at 12:10:12PM +0200, Martin Pieuchot wrote:
> On 18/08/20(Tue) 15:30, Visa Hankala wrote:
> > On Tue, Aug 18, 2020 at 11:04:47AM +0200, Martin Pieuchot wrote:
> > > Diff below changes the order of operations in kqueue_register() to get
> > > rid of an unnecessary pool_get().  When an event is already present on
> > > the list try to acquire it first.  Note that knote_acquire() may sleep
> > > in which case the list might have changed so the lookup has to always
> > > begin from the start.
> > > 
> > > This will help with lazy removal of knote in poll/select.  In this
> > > scenario EV_ADD is generally always done with an knote already on the
> > > list.
> > 
> > Some of the overhead could be absorbed by using a pool cache, as shown
> > in the diff below. However, I am not suggesting that the cache should
> > be taken into use right now. The frequency of knote pool usage is
> > relatively low currently; there are other pools that would benefit more
> > from caching.
> 
> Agreed, this is a nice idea to revisit.  Do you have a way to measure
> which pool could benefit from caches?

I think systat(1)'s pool view gives a starting point.

> > A related question is what implications the increased use of the pool
> > cache feature would have under memory pressure.
> 
> Do you have suggestion on how to measure this as well?  Could dt(4)
> probes or evtcount() help us? 

I do not have a specific tool in mind, just a vague question: "Can the
system start to perform poorly more easily than before?" However, only
experimentation will tell.



Re: Fewer pool_get() in kqueue_register()

2020-08-19 Thread Martin Pieuchot
On 18/08/20(Tue) 15:30, Visa Hankala wrote:
> On Tue, Aug 18, 2020 at 11:04:47AM +0200, Martin Pieuchot wrote:
> > Diff below changes the order of operations in kqueue_register() to get
> > rid of an unnecessary pool_get().  When an event is already present on
> > the list try to acquire it first.  Note that knote_acquire() may sleep
> > in which case the list might have changed so the lookup has to always
> > begin from the start.
> > 
> > This will help with lazy removal of knote in poll/select.  In this
> > scenario EV_ADD is generally always done with an knote already on the
> > list.
> 
> Some of the overhead could be absorbed by using a pool cache, as shown
> in the diff below. However, I am not suggesting that the cache should
> be taken into use right now. The frequency of knote pool usage is
> relatively low currently; there are other pools that would benefit more
> from caching.

Agreed, this is a nice idea to revisit.  Do you have a way to measure
which pool could benefit from caches?

I'm also not in a hurry.  That said I'd like to be able to re-use
descriptor that are already on the kqueue.  For per-thread kqueue there's
no possible race.  And since we'll need some sort of serialization to
unlock kevent(2) this could be built on top of it.

> A related question is what implications the increased use of the pool
> cache feature would have under memory pressure.

Do you have suggestion on how to measure this as well?  Could dt(4)
probes or evtcount() help us? 

> Index: kern/init_main.c
> ===
> RCS file: src/sys/kern/init_main.c,v
> retrieving revision 1.300
> diff -u -p -r1.300 init_main.c
> --- kern/init_main.c  16 Jun 2020 05:09:29 -  1.300
> +++ kern/init_main.c  18 Aug 2020 15:09:38 -
> @@ -71,6 +71,7 @@
>  #include 
>  #endif
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -148,7 +149,6 @@ void  crypto_init(void);
>  void db_ctf_init(void);
>  void prof_init(void);
>  void init_exec(void);
> -void kqueue_init(void);
>  void futex_init(void);
>  void taskq_init(void);
>  void timeout_proc_init(void);
> @@ -431,7 +431,9 @@ main(void *framep)
>   prof_init();
>  #endif
>  
> - mbcpuinit();/* enable per cpu mbuf data */
> + /* Enable per cpu data. */
> + mbcpuinit();
> + kqueue_init_percpu();
>  
>   /* init exec and emul */
>   init_exec();
> Index: kern/kern_event.c
> ===
> RCS file: src/sys/kern/kern_event.c,v
> retrieving revision 1.142
> diff -u -p -r1.142 kern_event.c
> --- kern/kern_event.c 12 Aug 2020 13:49:24 -  1.142
> +++ kern/kern_event.c 18 Aug 2020 15:09:38 -
> @@ -205,6 +205,12 @@ kqueue_init(void)
>   PR_WAITOK, "knotepl", NULL);
>  }
>  
> +void
> +kqueue_init_percpu(void)
> +{
> + pool_cache_init(_pool);
> +}
> +
>  int
>  filt_fileattach(struct knote *kn)
>  {
> Index: sys/event.h
> ===
> RCS file: src/sys/sys/event.h,v
> retrieving revision 1.44
> diff -u -p -r1.44 event.h
> --- sys/event.h   22 Jun 2020 13:14:32 -  1.44
> +++ sys/event.h   18 Aug 2020 15:09:38 -
> @@ -210,6 +210,8 @@ extern void   knote_activate(struct knote 
>  extern void  knote_remove(struct proc *p, struct knlist *list);
>  extern void  knote_fdclose(struct proc *p, int fd);
>  extern void  knote_processexit(struct proc *);
> +extern void  kqueue_init(void);
> +extern void  kqueue_init_percpu(void);
>  extern int   kqueue_register(struct kqueue *kq,
>   struct kevent *kev, struct proc *p);
>  extern int   filt_seltrue(struct knote *kn, long hint);