From 75cc19ebb4f936bb09dba8e2dcc8778bc6beda9f Mon Sep 17 00:00:00 2001 From: Gregory Nutt Date: Fri, 6 Jul 2018 19:49:05 -0600 Subject: [PATCH] net/tcp: Fix a deadlock condition that can occur when (1) all network logic runs on a single work queue, (1) TCP write buffering is enabled, and (2) we run out of IOBs. In this case, the TCP write buffering logic was blocking on iob_alloc() with the network locked. Since the network was locked, the device driver polls that would provide take the write buffer data and release the IOBs could not execute. This fixes the problem by unlocking the network lock while waiting for the IOBs. --- net/tcp/tcp_wrbuffer.c | 31 +++++++++++++++++++--- net/utils/net_lock.c | 60 +++++++++++++++++++++++++++++++++++++++++- net/utils/utils.h | 21 +++++++++++++++ 3 files changed, 107 insertions(+), 5 deletions(-) diff --git a/net/tcp/tcp_wrbuffer.c b/net/tcp/tcp_wrbuffer.c index f8d0a09de8..c0eb6bfe4f 100644 --- a/net/tcp/tcp_wrbuffer.c +++ b/net/tcp/tcp_wrbuffer.c @@ -40,7 +40,6 @@ ****************************************************************************/ #include -#if defined(CONFIG_NET) && defined(CONFIG_NET_TCP) && defined(CONFIG_NET_TCP_WRITE_BUFFERS) #if defined(CONFIG_DEBUG_FEATURES) && defined(CONFIG_NET_TCP_WRBUFFER_DEBUG) /* Force debug output (from this file only) */ @@ -59,8 +58,11 @@ #include #include +#include "utils/utils.h" #include "tcp/tcp.h" +#if defined(CONFIG_NET_TCP) && defined(CONFIG_NET_TCP_WRITE_BUFFERS) + /**************************************************************************** * Private Types ****************************************************************************/ @@ -159,8 +161,29 @@ FAR struct tcp_wrbuffer_s *tcp_wrbuffer_alloc(void) /* Now get the first I/O buffer for the write buffer structure */ - wrb->wb_iob = iob_alloc(false); - if (!wrb->wb_iob) + wrb->wb_iob = iob_tryalloc(false); + if (wrb->wb_iob == NULL) + { + unsigned int count; + int ret; + + /* There are no buffers available now. We will have to wait for one to + * become available. But let's not do that with the network locked. + */ + + ret = net_breaklock(&count); + wrb->wb_iob = iob_alloc(false); + if (ret >= 0) + { + net_restorelock(count); + } + } + + /* Did we get an IOB? We should always get one except under some really weird + * error conditions. + */ + + if (wrb->wb_iob == NULL) { nerr("ERROR: Failed to allocate I/O buffer\n"); tcp_wrbuffer_release(wrb); @@ -284,4 +307,4 @@ int tcp_wrbuffer_test(void) return ret; } -#endif /* CONFIG_NET && CONFIG_NET_TCP && CONFIG_NET_TCP_WRITE_BUFFERS */ +#endif /* CONFIG_NET_TCP && CONFIG_NET_TCP_WRITE_BUFFERS */ diff --git a/net/utils/net_lock.c b/net/utils/net_lock.c index 3bf9128ade..6b32062c5f 100644 --- a/net/utils/net_lock.c +++ b/net/utils/net_lock.c @@ -1,7 +1,7 @@ /**************************************************************************** * net/utils/net_lock.c * - * Copyright (C) 2011-2012, 2014-2017 Gregory Nutt. All rights reserved. + * Copyright (C) 2011-2012, 2014-2018 Gregory Nutt. All rights reserved. * Author: Gregory Nutt * * Redistribution and use in source and binary forms, with or without @@ -204,6 +204,64 @@ void net_unlock(void) #endif } +/**************************************************************************** + * Name: net_breaklock + * + * Description: + * Break the lock, return information needed to restore re-entrant lock + * state. + * + ****************************************************************************/ + +int net_breaklock(FAR unsigned int *count) +{ + irqstate_t flags; + pid_t me = getpid(); + int ret = -EPERM; + + DEBUGASSERT(count != NULL); + + flags = spin_lock_irqsave(); /* No interrupts */ + if (g_holder == me) + { + /* Return the lock setting */ + + *count = g_count; + + /* Release the network lock */ + + g_holder = NO_HOLDER; + g_count = 0; + + (void)nxsem_post(&g_netlock); + ret = OK; + } + + spin_unlock_irqrestore(flags); + return ret; +} + +/**************************************************************************** + * Name: net_breaklock + * + * Description: + * Restore the locked state + * + ****************************************************************************/ + +void net_restorelock(unsigned int count) +{ + pid_t me = getpid(); + + DEBUGASSERT(g_holder != me); + + /* Recover the network lock at the proper count */ + + _net_takesem(); + g_holder = me; + g_count = count; +} + /**************************************************************************** * Name: net_timedwait * diff --git a/net/utils/utils.h b/net/utils/utils.h index ddbbff1aa8..75d1dd8904 100644 --- a/net/utils/utils.h +++ b/net/utils/utils.h @@ -86,6 +86,27 @@ struct timeval; /* Forward reference */ void net_lockinitialize(void); +/**************************************************************************** + * Name: net_breaklock + * + * Description: + * Break the lock, return information needed to restore re-entrant lock + * state. + * + ****************************************************************************/ + +int net_breaklock(FAR unsigned int *count); + +/**************************************************************************** + * Name: net_breaklock + * + * Description: + * Restore the locked state + * + ****************************************************************************/ + +void net_restorelock(unsigned int count); + /**************************************************************************** * Name: net_dsec2timeval *