From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qc0-f169.google.com (mail-qc0-f169.google.com [209.85.216.169]) by dpdk.org (Postfix) with ESMTP id 7CD67377A for ; Tue, 7 Apr 2015 19:31:04 +0200 (CEST) Received: by qcyk17 with SMTP id k17so24398978qcy.1 for ; Tue, 07 Apr 2015 10:31:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=7DzpHSlgMcABkWfc5ZyhZ+hzxRSEssJPHj4p1/EuL1o=; b=IdCUj2cll7ub9w6zq+IfKRSjurlkTIzKF8OZlbsfAUWzUCTeTXotaBUA3a0ETRZD3I EepY7DdEjOKQRas/BFaicYV3ys8wqbWI0A38p48yjg0Nqb+OzQuo56Zw2cquLqporvCV YfjsUgDx2K7fq1yz7FaPebiRt1G1iRyix+9yzv4F1Yqg/Gajk0w0cBmHpr0idYfvWFkF 4VSwvmwybaIg0/MtDl9zXRvLw/ZeH1uT2D/Y457VerzHs16zNYL4YqAf7yaBGCy52hz5 SZNenu0Zb4uL/SN1dz2XlUL/0FZSh8b57eh/j2vFkK4ZFI4eYiiSt1kMMtxh/9bGzHM+ WIbA== X-Received: by 10.140.145.85 with SMTP id 82mr3876667qhr.32.1428427864074; Tue, 07 Apr 2015 10:31:04 -0700 (PDT) Received: from gmail.com (pool-108-31-208-15.washdc.fios.verizon.net. [108.31.208.15]) by mx.google.com with ESMTPSA id b81sm5753829qkb.38.2015.04.07.10.31.01 (version=TLSv1 cipher=RC4-SHA bits=128/128); Tue, 07 Apr 2015 10:31:03 -0700 (PDT) Received: by gmail.com (sSMTP sendmail emulation); Tue, 7 Apr 2015 13:31:01 -0400 Date: Tue, 7 Apr 2015 13:31:01 -0400 From: Eric Kinzie To: Pawel Wodkowski Message-ID: <20150407173055.GA24191@roosta.home> References: <1428339685-27686-1-git-send-email-ehkinzie@gmail.com> <1428339685-27686-5-git-send-email-ehkinzie@gmail.com> <5523E720.2050207@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5523E720.2050207@intel.com> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH 4/5] bond mode 4: allow external state machine X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 07 Apr 2015 17:31:04 -0000 On Tue Apr 07 16:18:08 +0200 2015, Pawel Wodkowski wrote: > On 2015-04-06 19:01, Eric Kinzie wrote: > > Interesting patch. I will closer look at this tomorrow. > > For now I have first comments: > > >+static void bond_mode_8023ad_ext_periodic_cb(void *arg); > >+ > > #ifdef RTE_LIBRTE_BOND_DEBUG_8023AD > > #define MODE4_DEBUG(fmt, ...) RTE_LOG(DEBUG, PMD, "%6u [Port %u: %s] " fmt, \ > > bond_dbg_get_time_diff_ms(), slave_id, \ > >@@ -1014,6 +1016,8 @@ bond_mode_8023ad_conf_get(struct rte_eth_dev *dev, > > conf->tx_period_ms = mode4->tx_period_timeout / ms_ticks; > > conf->update_timeout_ms = mode4->update_timeout_us / 1000; > > conf->rx_marker_period_ms = mode4->rx_marker_timeout / ms_ticks; > >+ conf->slowrx_cb = mode4->slowrx_cb; > >+ conf->external_sm = mode4->external_sm; > > mode4->external_sm flag realy needed? Why do not use > mode4->slowrx_cb as external state machine indicator? I'll remove the external_sm flag. You're correct, it's not needed. > > } > > > > void > >@@ -1035,6 +1039,8 @@ bond_mode_8023ad_setup(struct rte_eth_dev *dev, > > conf->tx_period_ms = BOND_8023AD_TX_MACHINE_PERIOD_MS; > > conf->rx_marker_period_ms = BOND_8023AD_RX_MARKER_PERIOD_MS; > > conf->update_timeout_ms = BOND_MODE_8023AX_UPDATE_TIMEOUT_MS; > >+ conf->slowrx_cb = NULL; > >+ conf->external_sm = 0; > > } > > > > mode4->fast_periodic_timeout = conf->fast_periodic_ms * ms_ticks; > >@@ -1045,6 +1051,8 @@ bond_mode_8023ad_setup(struct rte_eth_dev *dev, > > mode4->tx_period_timeout = conf->tx_period_ms * ms_ticks; > > mode4->rx_marker_timeout = conf->rx_marker_period_ms * ms_ticks; > > mode4->update_timeout_us = conf->update_timeout_ms * 1000; > >+ mode4->slowrx_cb = conf->slowrx_cb; > >+ mode4->external_sm = conf->external_sm; > > } > > > > int > >@@ -1062,6 +1070,13 @@ bond_mode_8023ad_enable(struct rte_eth_dev *bond_dev) > > int > > bond_mode_8023ad_start(struct rte_eth_dev *bond_dev) > > { > >+ struct bond_dev_private *internals = bond_dev->data->dev_private; > >+ struct mode8023ad_private *mode4 = &internals->mode4; > >+ > >+ if (mode4->external_sm) > >+ return rte_eal_alarm_set(BOND_MODE_8023AX_UPDATE_TIMEOUT_MS * 1000, > >+ &bond_mode_8023ad_ext_periodic_cb, bond_dev); > >+ > > return rte_eal_alarm_set(BOND_MODE_8023AX_UPDATE_TIMEOUT_MS * 1000, > > &bond_mode_8023ad_periodic_cb, bond_dev); > > } > >@@ -1069,6 +1084,13 @@ bond_mode_8023ad_start(struct rte_eth_dev *bond_dev) > > void > > bond_mode_8023ad_stop(struct rte_eth_dev *bond_dev) > > { > >+ struct bond_dev_private *internals = bond_dev->data->dev_private; > >+ struct mode8023ad_private *mode4 = &internals->mode4; > >+ > >+ if (mode4->external_sm) { > > This is bad idea. If bond_mode_8023ad_setup will be called you might > have two handlers running for while. You should stop mode 4 by > invoking bond_mode_8023ad_stop() before you set mode4->external_sm > and then, if mode 4 was running, start it again. Thanks, I'll make that change. > Also, maybe a renaming "external_sm" to "state_machine_cb", set it > to against default one and using it without "if()" will simplify > code. It is no crucial but will eliminate couple of if's. In > rte_eth_bond_8023ad_ext_slowtx() you can compare it against default > one. > > >+ rte_eal_alarm_cancel(&bond_mode_8023ad_ext_periodic_cb, bond_dev); > >+ return; > >+ } > > rte_eal_alarm_cancel(&bond_mode_8023ad_periodic_cb, bond_dev); > > } > > > >@@ -1215,3 +1237,156 @@ rte_eth_bond_8023ad_slave_info(uint8_t port_id, uint8_t slave_id, > > info->agg_port_id = port->aggregator_port_id; > > return 0; > > } > > -- > Pawel