Date: Fri, 4 Aug 2017 13:37:48 +0530
From: "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>
To: Vince Weaver <vincent.weaver@maine.edu>
Cc: Peter Zijlstra <peterz@infradead.org>, Jiri Olsa <jolsa@redhat.com>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Ingo Molnar <mingo@kernel.org>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/2] kernel/events: Add option to notify through
 signals on wakeup
References: <cover.1501576497.git.naveen.n.rao@linux.vnet.ibm.com>
 <ea1be1160ae17a5348560a5825960f517825e76b.1501576497.git.naveen.n.rao@linux.vnet.ibm.com>
 <alpine.DEB.2.20.1708031355400.26654@macbook-air>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <alpine.DEB.2.20.1708031355400.26654@macbook-air>
User-Agent: NeoMutt/20170714 (1.8.3)
Message-Id: <20170804080748.c3lxlsbim4tzg45y@naverao1-tp.localdomain>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1711
Lines: 43

Hi Vince,
Thanks for taking a look.

On 2017/08/03 01:57PM, Vince Weaver wrote:
> On Tue, 1 Aug 2017, Naveen N. Rao wrote:
> 
> > Add a new option 'signal_on_wakeup' to request for a signal to be
> > delivered on ring buffer wakeup controlled through watermark and
> > {wakeup_events, wakeup_watermark}. HUP is signaled on exit.
> > 
> > Setting signal_on_wakeup disables use of IOC_REFRESH to control signal
> > delivery, instead relying on IOC_ENABLE/DISABLE.
> 
> so I probably missed the original thread on this new interface, but why is 
> IOC_REFRESH not being used?

IOC_REFRESH is used to control the number of overflows before disabling 
the event. It works outside of perf_event_attr in the sense that it 
enables POLL_IN on each overflow and user specifies the number of 
overflows after which to disable the event as part of the ioctl (when 
HUP is signaled).

However, signal_on_wakeup is designed to work with the values in the 
perf_event_attr structure itself. wakeup_events controls the number of 
events after which to signal POLL_IN. signal_on_wakeup itself needs to 
be specified in the perf_event_attr. As such, I felt it is better to 
have all control through perf_event_attr.

But, if you think having IOC_REFRESH available in this scenario is 
useful, we can revisit this. Ideally, we would have separate ioctls to 
control signal delivery separate from perf_event_attr, but I am not sure 
how useful that would be.

> 
> For new interfaces like this it's also nice to have some text that can be 
> added to the perf_event_open() manpage, especially if there's weird 
> conditions like this.

Sure -- I will send an update to the manpage once this series gets 
accepted.

Thanks,
Naveen