Index | Thread | Search

From:
Martin Pieuchot <mpi@openbsd.org>
Subject:
Re: dt(4), hardclock: move profiling entry points to dedicated clockintrs
To:
Scott Cheloha <scottcheloha@gmail.com>
Cc:
tech@openbsd.org
Date:
Thu, 8 Feb 2024 17:19:40 +0100

Download raw body.

Thread
On 07/02/24(Wed) 20:22, Scott Cheloha wrote:
> On Tue, Feb 06, 2024 at 03:24:20PM +0100, Martin Pieuchot wrote:
> > On 05/02/24(Mon) 19:37, Scott Cheloha wrote:
> > > On Fri, Jan 26, 2024 at 12:28:12PM -0600, Scott Cheloha wrote:
> > > > This patch moves the dt(4) profiling entry points from hardclock()
> > > > into dedicated clockintr callbacks.  It depends on the clockintr_unbind
> > > > patch I sent out separately, so be sure to apply that first.
> > > > 
> > > > [...]
> > > 
> > > Updated patch:
> > > 
> > > - Don't advance/cancel dp_clockintr when dp_nsecs is zero.
> > > - Update DT_FA_PROFILE for i386, macppc
> > 
> > I can't test those.
> [...]
> We can combine them, but in multiple previous emails you have
> requested that they remain separate.

Because they were reasons to do so which are now gone.

> So, which do you want?  Either works for me.

Merge them.

> > [...] 
> > Here this is complex.  Since we are no longer using SMR in the callback
> > to get a reference to `dp' we should not do the following before we are
> > sure the callback won't run.  That would imply clockintr_unbind(), no?
> > Some caution should be taken with clockintr_unbind() because we are
> > holding a lock.
> > 
> > >  		dtp->dtp_recording--;
> > >  		dtp->dtp_prov->dtpv_recording--;
> > >  		SMR_SLIST_REMOVE_LOCKED(&dtp->dtp_pcbs, dp, dt_pcb, dp_pnext);
> 
> We should not unbind here.  All we need to do is cancel dp_clockintr.

No, we must ensure no other CPU has a valid reference on the descriptor
before decrementing the counter.   That's what I already said in my
previous email in different words.

> With the patch, dt_prov_profile_clock() doesn't walk the SMR list
> (dtp_pcbs) anymore and it doesn't check dtpv_recording or
> dtp_recording anymore, so we don't need to do anything but
> advance/cancel dp_clockintr when starting/stopping recording when
> dp_nsecs is zero.  Check the updated patch.  Is that acceptable?

No it's not.  

> > [...]
> > This is too late and should not be necessary.  dtclose() calls 
> > ctl_record_stop() which should take care of the barrier before
> > decrementing counters.
> 
> I don't know what you mean by "too late".  Too late for what?

I don't want to explain.  It doesn't work with you.  You want to push
your way of doing things, I'm tired of wasting my time.  I already said
what I had to say in the previous email.  Your email is a joke to waste
my time.  I'm tired and angry.