Re: Reduce contention on fstrans_lock

[Taylor R Campbell <campbell+netbsd-tech-kern%mumble.net@localhost>]

> Date: Sat, 4 Apr 2020 14:37:44 -0700
> From: Jason Thorpe <thorpej%me.com@localhost>
> 
> +static int
> +fstrans_lwp_ctor(void *arg, void *obj, int flags)
> +{
> +	struct fstrans_lwp_info *fli = obj;
> +
> +	memset(fli, 0, sizeof(*fli));
> +	membar_sync();
> +	mutex_enter(&fstrans_lock);
> +	LIST_INSERT_HEAD(&fstrans_fli_head, fli, fli_list);
> +	mutex_exit(&fstrans_lock);
> +	return 0;
> +}
> 
> That membar_sync() could be a membar_producer().

Unless there are shenanigans about using fstrans_fli_head or struct
fstrans_lwp_info::fli_list outside fstrans_lock, there is no need for
this membar at all and it should be removed.

+       fli = pool_cache_get(fstrans_lwp_cache, PR_WAITOK);
+       KASSERT(fli->fli_self == NULL);     
+       KASSERT(fli->fli_mountinfo == NULL);
+       KASSERT(fli->fli_trans_cnt == 0);
+       KASSERT(fli->fli_cow_cnt == 0);  
+       KASSERT(fli->fli_alias_cnt == 0); 
+       fli->fli_succ = curlwp->l_fstrans;
+       curlwp->l_fstrans = fli;
+       membar_sync();
+       fli->fli_self = curlwp;

I am not clear on what the ordering here is supposed to be about.

There appear to be only four places where we read fli_self:

1. fstrans_lwp_dtor (renamed fstrans_lwp_free), where we
   KASSERT(fli->fli_self == l) -- there are no corresponding membars
   around this, which is correct because it's supposed to happen in
   the same lwp which sees its own memory operations in program order

2. fstrans_clear_lwp_info, same

3. fstrans_alloc_lwp_info -- replaced by pool_cache_get, no longer
   relevant

4. state_change_done -- although this takes a data-dependent control
   flow based on fli->fli_self, there are no ordering barriers here;
   perhaps it's a bug that they're missing, or perhaps they're not
   needed, and none of the preceding writes (fli->fli_succ,
   curlwp->l_fstrans) affects state_change_done (fli->fli_mountinfo,
   fli->fli_trans_cnt, fli->fli_lock_tpye)

5. fstrans_print_lwp -- debugging routine for ddb, memory ordering is
   issue-and-pray here

So I don't see why we have this membar_sync.

Either we should document it and identify the membar that it pairs
with, if it is really needed, or discard it, if -- as I suspect -- it
is spurious.  For that matter, I don't understand the purpose of any
of the membars in this file, actually!