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