Questions on the correct Asynchronous Event Request Interface

[Indraneel Mukherjee]

> -----Original Message-----
> From: Linux-nvme [mailto:linux-nvme-bounces at lists.infradead.org] On Behalf
> Of Keith Busch
> Sent: Thursday, June 12, 2014 9:37 PM
> To: INDRANEEL MUKHERJEE
> Cc: linux-nvme at lists.infradead.org
> Subject: Re: Questions on the correct Asynchronous Event Request Interface
> 
> On Wed, 11 Jun 2014, INDRANEEL MUKHERJEE wrote:
> > A couple of patches have already been posted on the Asynchronous Event
> > Request(AER) admin command.  But there is still no clarity on the what
> > is the correct interface to the application.
> >
> > Here'r some questions to help summarise the requirements & finalise an
> > interface so that a patch can follow.  (References to kfifo are based
> > on Keith's patch -
> > http://lists.infradead.org/pipermail/linux-nvme/2013-August/000350.htm
> > l )
> >
> > 1. Should the Driver return both the Async Event Completion & the
> > corresponding GetLogPage together to User Space?
> >
> > This can help to avoid the Async Event getting masked accidently. It's
> > possible that any Application reads the Async Event from kfifo but
> > fails to read the corresponding Log Page masking the event indefinitely.
> 
> I don't think so. The driver has no gaurantee there is even a user space
app
> consuming these events. If the driver is unmasking them, we could continue
> getting events and overrun older ones.
> 

What I meant was that when application issues a read for reading the kfifo
event, the driver can issue the corresponding get_log_page and return both
the kfifo event and the corresponding log page together.
Application should ensure it passes a large enough buffer for this. This
will ensure that event is not masked accidently.

> > 2. How to have provision in driver to set the 'Temperature Threshold'
> > & 'Asynchronous Event Configuration' before sending the AER command?
> >
> > This is needed as Spec says these should be set before the Async Event
> > Request command is sent.
> 
> Does it? Please point me to the section in the spec, because I'm not
finding it in
> either 5.2 or 5.12.1.11.
> 
> Section 5.12.1.4 for Temperature Threshold feature says:
> 
> "The host should configure this feature prior to enabling asynchronous
event
> notification for the temperature exceeding the threshold".
> 
> I take that to mean the host should set the asynchronous event
configuration
> feature to enable temperature notification after you set the temperature
> threshold. All this can be done from user space and async event requests
may be
> outstanding prior to this without affecting it.

I tested this on the device. Works as you say.

> 
> > One way to achieve this is by making these module parameters and doing
> > a SetFeature during Probe & Resume operations. If these module
> > parameters are provided by user during insmod, driver should ensure
> > that these are set before the kthread submits the AER.
> 
> What if I have different devices with vastly different temperature
thresholds in
> my machine?
> 
> > 3. Should the contents kfifo be preserved when the system goes into
Suspend?
> 
> Maybe not. I don't think they would be relevant anymore after a resume.
> 
> > 4. Only one application will get any event (kfifo ensures this). Is
> > this expected?
> 
> That's how it's designed, but I didn't get feedback if that's okay. It's
readable
> only by root, so the expectation is you know what you're doing. Any other
> suggestions are appreciated, though.


Noticed one thing in the kfifo patch - it uses the following sequence:
spin_lock(&dev->event_lock);
kfifo_to_user()       ----->  Can sleep as it is a copy_to_user() call
internally  <------
spin_unlock(&dev->event_lock)


> 
> > 5. How to have provision in the driver to cancel the current AER cmds
> > and send new ones after doing some new Set Feature?
> 
> But the controller shouldn't require a new async event request in order to
> respond to changed async configuration features.

Agreed. I had misinterpreted.

> 
> > This will need the CmdIds for current AREs to be exported to User
> > Space so that they can be aborted. Should the Driver support this?
> >
> > 6. Is the IOCtl interface enough? Or we want to keep the Read/Poll
interface?
> 
> Is the IOCTL interface enough for what? It's certainly not usable for
async event
> notification as-is. You want to add a new IOCTL to consume aysnc events
> instead of getting them from a read?
> 
> > 7. Is there any reference utility that can be looked at to see what
> > such monitoring applications expect?
> 
> That would be something I'd like to see as well. I've no idea how these
sorts of
> events are typically consumed so I just made something up.
> 
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme