Re: [dpdk-dev] [EXT] Re: [PATCH v2 1/6] eal/interrupts: implement get set APIs

[Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>]

2021-10-14 17:15 (UTC+0000), Harman Kalra:
> [...]
> > > +int rte_intr_fd_get(const struct rte_intr_handle *intr_handle) {
> > > +	if (intr_handle == NULL) {
> > > +		RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
> > > +		rte_errno = ENOTSUP;
> > > +		goto fail;
> > > +	}
> > > +
> > > +	return intr_handle->fd;
> > > +fail:
> > > +	return -1;
> > > +}  
> > 
> > Please add a similar pair of experimental API for the "handle" member, it is
> > needed for Windows interrupt support I'm working on top of these series
> > (IIUC, API changes should be closed by RC1.) If you will be doing this and
> > don't like "handle" name, it might be like "dev_handle" or
> > "windows_device".  
> 
> I add new APIs to get/set handle. Let's rename it to "windows_handle"

The name works for me, thanks.
 
> > > [...]
> > > +int rte_intr_max_intr_set(struct rte_intr_handle *intr_handle,
> > > +				 int max_intr)
> > > +{
> > > +	if (intr_handle == NULL) {
> > > +		RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
> > > +		rte_errno = ENOTSUP;
> > > +		goto fail;
> > > +	}
> > > +
> > > +	if (max_intr > intr_handle->nb_intr) {
> > > +		RTE_LOG(ERR, EAL, "Max_intr=%d greater than
> > > +RTE_MAX_RXTX_INTR_VEC_ID=%d",  
> > 
> > The macros is not used in the comparison, neither should the log mention it.  
> 
> I will add the check.

What check? I mean that the condition is `max_intr > intr_handle->nb_intr`,
so `RTE_MAX_RXTX_INTR_VEC_ID` is not relevant, `intr_handle->nb_intr` is
dynamic. Probably it should be like this:

	RTE_LOG(ERR, EAL, "Maximum interrupt vector ID (%d) exceeds "
		"the number of available events (%d)\n",
		max_intr, intr_handle->nb_intr);

> [...] 
> > > +	rte_intr_type_set;
> > > +	rte_intr_type_get;
> > > +	rte_intr_instance_alloc;
> > > +	rte_intr_instance_free;
> > >  };  
> > 
> > Do I understand correctly that these exports are needed to allow an
> > application to use DPDK callback facilities for its own interrupt sources?  
> 
> I exported only those APIs which are currently used by test suite or example
> applications, may be later more APIs can be moved from internal to public on
> need basis.
> 
> 
> > If so, I'd suggest that instead we export a simpler set of functions:
> > 1. Create/free a handle instance with automatic fixed type selection.
> > 2. Trigger an interrupt on the specified handle instance.
> > The flow would be that the application listens on whatever it wants, probably
> > with OS-specific mechanisms, and just notifies the interrupt thread about
> > events to trigger callbacks.
> > Because these APIs are experimental we don't need to change it now, just my
> > thoughts for the future.  
> 
> I am sorry but I did not followed your suggestion, can you please explain.

These API is used as follows. The application has a file descriptor
that becomes readable on some event. The programmer doesn't want to create
another thread like EAL interrupt thread, implement thread-safe callback
registration and invocation. They want to reuse DPDK mechanism instead.
So they create an instance of type EXT and give it the descriptor.
In case of the unit test the descriptor is a pipe read end.
In case of a real application it can be a socket, like in mlx5 PMD.
This is often convenient, but not always. An event may be a signal,
or busy-wait end, or it may be Windows with its completely different IO model
(it's "issue an IO, wait for completion" instead of POSIX
"wait for IO readiness, do a blocking IO").
In all these cases the user needs to create a fake pipe (or whatever)
to fit into how the interrupt thread waits for events.
But what the application really needs is to say "there's an event, please run
the callback on this handle". It's a function call that doesn't require any
explicit file descriptors or handles, doesn't rely on any IO model.
How it is implemented depends on the EAL, for POSIX it will probably be
an internal pipe, Windows can use APC as in eal_intr_thread_schedule().
Again, I'm thinking out loud here, nothing of this needs to be done now.