Re: [PATCH v2 2/2] eal/linux: handle interrupt epoll events

[Kevin Traynor <ktraynor@redhat.com>]

On 10/02/2026 09:17, David Marchand wrote:
> Hello Kevin,
> 
> On Fri, 6 Feb 2026 at 18:21, Kevin Traynor <ktraynor@redhat.com> wrote:
>>
>> Add handling for epoll error and disconnect conditions EPOLLERR,
>> EPOLLHUP and EPOLLRDHUP.
>>
>> These events indicate that the interrupt file descriptor is in
>> an error state or there has been a hangup.
>>
>> Only do this for interrupts that are read in eal. Interrupts that
>> are read outside eal should deal with different interrupt scenarios
>> appropriate to their functionality. e.g. virtio interrupt handling
>> has reconnect mechanisms for some cases.
>>
>> Also, treat no bytes read as an error condition.
>>
>> Bugzilla ID: 1873
>> Fixes: af75078fece3 ("first public release")
>> Cc: stable@dpdk.org
> 
> Cc: Harman.
> 
>>
>> Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
>> ---
>>  lib/eal/linux/eal_interrupts.c | 67 ++++++++++++++++++++++------------
>>  1 file changed, 44 insertions(+), 23 deletions(-)
>>
>> diff --git a/lib/eal/linux/eal_interrupts.c b/lib/eal/linux/eal_interrupts.c
>> index 9db978923a..68ca0f929e 100644
>> --- a/lib/eal/linux/eal_interrupts.c
>> +++ b/lib/eal/linux/eal_interrupts.c
>> @@ -887,4 +887,26 @@ rte_intr_disable(const struct rte_intr_handle *intr_handle)
>>  }
>>
>> +static void
>> +eal_intr_source_remove_and_free(struct rte_intr_source *src)
>> +{
>> +       struct rte_intr_callback *cb, *next;
>> +
>> +       /* Remove the interrupt source */
>> +       rte_spinlock_lock(&intr_lock);
>> +       TAILQ_REMOVE(&intr_sources, src, next);
>> +       rte_spinlock_unlock(&intr_lock);
>> +
>> +       /* Free callbacks */
>> +       for (cb = TAILQ_FIRST(&src->callbacks); cb; cb = next) {
>> +               next = TAILQ_NEXT(cb, next);
>> +               TAILQ_REMOVE(&src->callbacks, cb, next);
>> +               free(cb);
>> +       }
>> +
>> +       /* Free the interrupt source */
>> +       rte_intr_instance_free(src->intr_handle);
>> +       free(src);
>> +}
>> +
>>  static int
>>  eal_intr_process_interrupts(struct epoll_event *events, int nfds)
>> @@ -952,4 +974,16 @@ eal_intr_process_interrupts(struct epoll_event *events, int nfds)
>>
>>                 if (bytes_read > 0) {
>> +                       /**
>> +                        * Check for epoll error or disconnect events for
>> +                        * interrupts that are read directly in eal.
>> +                        */
>> +                       if (events[n].events & (EPOLLERR | EPOLLHUP | EPOLLRDHUP)) {
>> +                               EAL_LOG(INFO, "Disconnect condition on fd %d "
> 
> This is an anormal situation, I would make this log level the same as
> other logs below.
> 
> The fact that the interrupt fd gets into this state should be
> something to report and investigate.
> 

ok. I'll change to warning.

> 
>> +                                       "(events=0x%x), removing from epoll",
>> +                                       events[n].data.fd, events[n].events);
>> +                               eal_intr_source_remove_and_free(src);
>> +                               return -1;
>> +                       }
>> +
>>                         /**
>>                          * read out to clear the ready-to-be-read flag
>> @@ -957,5 +991,7 @@ eal_intr_process_interrupts(struct epoll_event *events, int nfds)
>>                          */
>>                         bytes_read = read(events[n].data.fd, &buf, bytes_read);
>> -                       if (bytes_read < 0) {
>> +                       if (bytes_read > 0) {
>> +                               call = true;
>> +                       } else if (bytes_read < 0) {
>>                                 if (errno == EINTR || errno == EWOULDBLOCK)
>>                                         continue;
>> @@ -965,27 +1001,12 @@ eal_intr_process_interrupts(struct epoll_event *events, int nfds)
>>                                         events[n].data.fd,
>>                                         strerror(errno));
>> -                               /*
>> -                                * The device is unplugged or buggy, remove
>> -                                * it as an interrupt source and return to
>> -                                * force the wait list to be rebuilt.
>> -                                */
>> -                               rte_spinlock_lock(&intr_lock);
>> -                               TAILQ_REMOVE(&intr_sources, src, next);
>> -                               rte_spinlock_unlock(&intr_lock);
>> -
>> -                               for (cb = TAILQ_FIRST(&src->callbacks); cb;
>> -                                                       cb = next) {
>> -                                       next = TAILQ_NEXT(cb, next);
>> -                                       TAILQ_REMOVE(&src->callbacks, cb, next);
>> -                                       free(cb);
>> -                               }
>> -                               rte_intr_instance_free(src->intr_handle);
>> -                               free(src);
>> -                               return -1;
>> -                       } else if (bytes_read == 0)
>> -                               EAL_LOG(ERR, "Read nothing from file "
>> +                       } else { /* bytes == 0 */
> 
> "bytes_read == 0", or remove this comment as the code is quite compact
> and leaves little space for wondering what this else block is about.
> 

Ack. I will take it as a compliment and remove the comment ;-)

> 
>> +                               EAL_LOG(WARNING, "Read nothing from file "
> 
> I would keep this log at the same level than the < 0 condition.
> It seems the same type of error.
> 
>>                                         "descriptor %d", events[n].data.fd);
> 
> And avoid splitting the format string.
> 

Ack.

> 
>> -                       else
>> -                               call = true;
>> +                       }
>> +                       if (bytes_read <= 0) {
>> +                               eal_intr_source_remove_and_free(src);
>> +                               return -1;
>> +                       }
>>                 }
>>
>> --
>> 2.52.0
>>
> 
> Except those nits, the fix looks correct.
> 
> Acked-by: David Marchand <david.marchand@redhat.com>
> 


Thanks David. I will make these changes in v3.

> 
> 
>