From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 42C4241C5B for ; Fri, 10 Feb 2023 07:47:50 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 30970427F2; Fri, 10 Feb 2023 07:47:50 +0100 (CET) Received: from mail-vk1-f174.google.com (mail-vk1-f174.google.com [209.85.221.174]) by mails.dpdk.org (Postfix) with ESMTP id 63E6F40EE3; Fri, 10 Feb 2023 07:47:47 +0100 (CET) Received: by mail-vk1-f174.google.com with SMTP id o28so2180650vkf.12; Thu, 09 Feb 2023 22:47:47 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=sOh431CabNJAYT2Fa7HIU7Z9byX+ygAWNDKb738kitk=; b=NSXZzLyAIZS8N9mbnsnuUXwnv7g/CcX1XKoEKSid3moDw0YLRLI2Uc2pJy+ruUm3Kg sPFDAIjC16xnPNDJjYkB4+IPh2mE/pmGgt2TcwfMzZoOGsqVS0hUCvckTZ9yRbuLVfjF p1QiR7OdF+Ykpp6yE5ACDfAEcRYO83lM0k6H7JuIK/+IcYDqm95cqp9U50S67HoBlgM4 tegOUdd/5QHG2J0vtiR2ZzoR5okvVcKWhbM5ubJzJWElEmccc8P3VsTfxG4oa0Is3ECW IhaOg3mU2W9iHLnOIbxcxZGthYVfKbFnTom7Yrx+HC8LgYx00UFozQsWyHxfcfuwYK4S f5Kw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=sOh431CabNJAYT2Fa7HIU7Z9byX+ygAWNDKb738kitk=; b=va92+nZkYh9fwFICAUj7sNTvlDzIQtx9CNdyIXg8ldUcSmF6Nnj0AOMO1kCLBPBc/R GQn2KhOi4cMHlBl6N2yH6OD8fFGk/O7JQOWCFLY0dvGwZiXQK+rykQrojTjTx0Kr+ryj Q6UGFWa2Ljv+kUWEpvbMWOCy3MdFdKc6NLVM5uwYV1Juw7m1H389IVqaJ7DGQnbDnilH Pw6uvHvAl+n0xjxb2OnpXDCiom/O5bVfhpe4ECnlS2P9N+LL4j8kB+jSXu6Fd6UHS+/1 xYsJLQGRmIShg6J+P+BihBL9Ll+AXnE0lN+E+pGNb/hV/HNrPUFZByD8pMCo9goviVDQ 6dsw== X-Gm-Message-State: AO0yUKUxIehJTswnmpY9PmcalsSaTyDVp7zI/4GIBErBzrn3HzkH9pMX VkPTsysniiG4AiV4rRYrZjx3BAJpjuC+88a4/sA= X-Google-Smtp-Source: AK7set9joDl6qY83JwwxZ9U5Uy3dypGWAmljlDHGfYztsuxN6Ad6XeLdSGsZLw9KGk2K8TakOW3FwuKIxvgePxGpgDw= X-Received: by 2002:a1f:90d5:0:b0:401:14db:fa58 with SMTP id s204-20020a1f90d5000000b0040114dbfa58mr677263vkd.5.1676011666630; Thu, 09 Feb 2023 22:47:46 -0800 (PST) MIME-Version: 1.0 References: <20230124204555.3022361-1-erik.g.carrillo@intel.com> <20230209151349.474358-1-erik.g.carrillo@intel.com> In-Reply-To: <20230209151349.474358-1-erik.g.carrillo@intel.com> From: Jerin Jacob Date: Fri, 10 Feb 2023 12:17:20 +0530 Message-ID: Subject: Re: [PATCH v3] eventdev/timer: fix overflow issue To: Erik Gabriel Carrillo Cc: jerinj@marvell.com, stephen@networkplumber.org, dev@dpdk.org, stable@dpdk.org Content-Type: text/plain; charset="UTF-8" X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: stable-bounces@dpdk.org On Thu, Feb 9, 2023 at 8:44 PM Erik Gabriel Carrillo wrote: > > The software timer adapter converts event timer timeout ticks to a > number of TSC cycles at which an rte_timer should expire. The > computation uses an integer multiplication that can result in overflow. > > If necessary, reduce the timeout_nsecs value by the number of whole > seconds it contains to keep the value of the multiplier within a > range that will not result in overflow. Add the saved value back later > to get the final result. Also, move the logic that checks the timeout > range into the function that performs the above computation. > > Fixes: 6750b21bd6af ("eventdev: add default software timer adapter") > Cc: stable@dpdk.org > > Signed-off-by: Erik Gabriel Carrillo Applied to dpdk-next-net-eventdev/for-main. Thanks > --- > v3: > * Use integer operations instead of floating point, and use > rte_reciprocal_divide() for division. > > v2: > * Fix implicit int to float conversion build warning on Clang > > lib/eventdev/rte_event_timer_adapter.c | 97 ++++++++++++++++---------- > 1 file changed, 59 insertions(+), 38 deletions(-) > > diff --git a/lib/eventdev/rte_event_timer_adapter.c b/lib/eventdev/rte_event_timer_adapter.c > index 7f4f347369..23eb1d4a7d 100644 > --- a/lib/eventdev/rte_event_timer_adapter.c > +++ b/lib/eventdev/rte_event_timer_adapter.c > @@ -18,6 +18,7 @@ > #include > #include > #include > +#include > > #include "event_timer_adapter_pmd.h" > #include "eventdev_pmd.h" > @@ -734,13 +735,51 @@ swtim_callback(struct rte_timer *tim) > } > } > > -static __rte_always_inline uint64_t > +static __rte_always_inline int > get_timeout_cycles(struct rte_event_timer *evtim, > - const struct rte_event_timer_adapter *adapter) > + const struct rte_event_timer_adapter *adapter, > + uint64_t *timeout_cycles) > { > - struct swtim *sw = swtim_pmd_priv(adapter); > - uint64_t timeout_ns = evtim->timeout_ticks * sw->timer_tick_ns; > - return timeout_ns * rte_get_timer_hz() / NSECPERSEC; > + static struct rte_reciprocal_u64 nsecpersec_inverse; > + static uint64_t timer_hz; > + uint64_t rem_cycles, secs_cycles = 0; > + uint64_t secs, timeout_nsecs; > + uint64_t nsecpersec; > + struct swtim *sw; > + > + sw = swtim_pmd_priv(adapter); > + nsecpersec = (uint64_t)NSECPERSEC; > + > + timeout_nsecs = evtim->timeout_ticks * sw->timer_tick_ns; > + if (timeout_nsecs > sw->max_tmo_ns) > + return -1; > + if (timeout_nsecs < sw->timer_tick_ns) > + return -2; > + > + /* Set these values in the first invocation */ > + if (!timer_hz) { > + timer_hz = rte_get_timer_hz(); > + nsecpersec_inverse = rte_reciprocal_value_u64(nsecpersec); > + } > + > + /* If timeout_nsecs > nsecpersec, decrease timeout_nsecs by the number > + * of whole seconds it contains and convert that value to a number > + * of cycles. This keeps timeout_nsecs in the interval [0..nsecpersec) > + * in order to avoid overflow when we later multiply by timer_hz. > + */ > + if (timeout_nsecs > nsecpersec) { > + secs = rte_reciprocal_divide_u64(timeout_nsecs, > + &nsecpersec_inverse); > + secs_cycles = secs * timer_hz; > + timeout_nsecs -= secs * nsecpersec; > + } > + > + rem_cycles = rte_reciprocal_divide_u64(timeout_nsecs * timer_hz, > + &nsecpersec_inverse); > + > + *timeout_cycles = secs_cycles + rem_cycles; > + > + return 0; > } > > /* This function returns true if one or more (adapter) ticks have occurred since > @@ -774,23 +813,6 @@ swtim_did_tick(struct swtim *sw) > return false; > } > > -/* Check that event timer timeout value is in range */ > -static __rte_always_inline int > -check_timeout(struct rte_event_timer *evtim, > - const struct rte_event_timer_adapter *adapter) > -{ > - uint64_t tmo_nsec; > - struct swtim *sw = swtim_pmd_priv(adapter); > - > - tmo_nsec = evtim->timeout_ticks * sw->timer_tick_ns; > - if (tmo_nsec > sw->max_tmo_ns) > - return -1; > - if (tmo_nsec < sw->timer_tick_ns) > - return -2; > - > - return 0; > -} > - > /* Check that event timer event queue sched type matches destination event queue > * sched type > */ > @@ -1210,21 +1232,6 @@ __swtim_arm_burst(const struct rte_event_timer_adapter *adapter, > break; > } > > - ret = check_timeout(evtims[i], adapter); > - if (unlikely(ret == -1)) { > - __atomic_store_n(&evtims[i]->state, > - RTE_EVENT_TIMER_ERROR_TOOLATE, > - __ATOMIC_RELAXED); > - rte_errno = EINVAL; > - break; > - } else if (unlikely(ret == -2)) { > - __atomic_store_n(&evtims[i]->state, > - RTE_EVENT_TIMER_ERROR_TOOEARLY, > - __ATOMIC_RELAXED); > - rte_errno = EINVAL; > - break; > - } > - > if (unlikely(check_destination_event_queue(evtims[i], > adapter) < 0)) { > __atomic_store_n(&evtims[i]->state, > @@ -1240,7 +1247,21 @@ __swtim_arm_burst(const struct rte_event_timer_adapter *adapter, > evtims[i]->impl_opaque[0] = (uintptr_t)tim; > evtims[i]->impl_opaque[1] = (uintptr_t)adapter; > > - cycles = get_timeout_cycles(evtims[i], adapter); > + ret = get_timeout_cycles(evtims[i], adapter, &cycles); > + if (unlikely(ret == -1)) { > + __atomic_store_n(&evtims[i]->state, > + RTE_EVENT_TIMER_ERROR_TOOLATE, > + __ATOMIC_RELAXED); > + rte_errno = EINVAL; > + break; > + } else if (unlikely(ret == -2)) { > + __atomic_store_n(&evtims[i]->state, > + RTE_EVENT_TIMER_ERROR_TOOEARLY, > + __ATOMIC_RELAXED); > + rte_errno = EINVAL; > + break; > + } > + > ret = rte_timer_alt_reset(sw->timer_data_id, tim, cycles, > type, lcore_id, NULL, evtims[i]); > if (ret < 0) { > -- > 2.23.0 >