From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.tuxdriver.com (charlotte.tuxdriver.com [70.61.120.58]) by dpdk.org (Postfix) with ESMTP id DD094B3BF for ; Wed, 17 Sep 2014 16:45:33 +0200 (CEST) Received: from nat-pool-rdu-u.redhat.com ([66.187.233.203] helo=localhost) by smtp.tuxdriver.com with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.63) (envelope-from ) id 1XUGZY-0000yf-79; Wed, 17 Sep 2014 10:51:14 -0400 Date: Wed, 17 Sep 2014 10:51:02 -0400 From: Neil Horman To: Pawel Wodkowski Message-ID: <20140917145102.GC4213@localhost.localdomain> References: <1410963713-13837-1-git-send-email-pawelx.wodkowski@intel.com> <1410963713-13837-2-git-send-email-pawelx.wodkowski@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1410963713-13837-2-git-send-email-pawelx.wodkowski@intel.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-Spam-Score: -2.9 (--) X-Spam-Status: No Cc: dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH 1/2] bond: extract common code to separate functions 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: Wed, 17 Sep 2014 14:45:34 -0000 On Wed, Sep 17, 2014 at 03:21:52PM +0100, Pawel Wodkowski wrote: > Signed-off-by: Pawel Wodkowski > Reviewed-by: Declan Doherty > --- > lib/librte_pmd_bond/rte_eth_bond_api.c | 59 +++++++++++++++++++++------- > lib/librte_pmd_bond/rte_eth_bond_pmd.c | 47 ++++++++++++++-------- > lib/librte_pmd_bond/rte_eth_bond_private.h | 30 ++++++++++++-- > 3 files changed, 102 insertions(+), 34 deletions(-) > > diff --git a/lib/librte_pmd_bond/rte_eth_bond_api.c b/lib/librte_pmd_bond/rte_eth_bond_api.c > index dd33119..460df65 100644 > --- a/lib/librte_pmd_bond/rte_eth_bond_api.c > +++ b/lib/librte_pmd_bond/rte_eth_bond_api.c > @@ -31,6 +31,8 @@ > * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > */ > > +#include > + > #include > #include > #include > @@ -104,6 +106,39 @@ valid_slave_port_id(uint8_t port_id) > return 0; > } > > +void > +rte_eth_bond_activate_slave(struct rte_eth_dev *eth_dev, uint8_t port_id ) > +{ > + struct bond_dev_private *internals = eth_dev->data->dev_private; > + uint8_t active_count = internals->active_slave_count; > + > + internals->active_slaves[active_count] = port_id; > + > + > + internals->active_slave_count = active_count + 1; > +} > + > +void > +rte_eth_bond_deactive_slave(struct rte_eth_dev *eth_dev, I think you intended for this to be deactivate, not deactive, yes? > + uint8_t slave_pos ) > +{ > + struct bond_dev_private *internals = eth_dev->data->dev_private; > + uint8_t active_count = internals->active_slave_count; > + > + active_count--; > + > + /* If slave was not at the end of the list > + * shift active slaves up active array list */ > + if (slave_pos < active_count) { > + memmove(internals->active_slaves + slave_pos, > + internals->active_slaves + slave_pos + 1, > + (active_count - slave_pos) * > + sizeof(internals->active_slaves[0])); > + } > + > + internals->active_slave_count = active_count; > +} > + > uint8_t > number_of_sockets(void) Static? > { > @@ -356,10 +391,8 @@ rte_eth_bond_slave_add(uint8_t bonded_port_id, uint8_t slave_port_id) > if (bonded_eth_dev->data->dev_started) { > rte_eth_link_get_nowait(slave_port_id, &link_props); > > - if (link_props.link_status == 1) { > - internals->active_slaves[internals->active_slave_count++] = > - slave_port_id; > - } > + if (link_props.link_status == 1) > + rte_eth_bond_activate_slave(bonded_eth_dev, slave_port_id); > } > > return 0; > @@ -373,6 +406,7 @@ err_add: > int > rte_eth_bond_slave_remove(uint8_t bonded_port_id, uint8_t slave_port_id) > { > + struct rte_eth_dev *eth_dev; > struct bond_dev_private *internals; > struct slave_conf *slave_conf; > > @@ -386,20 +420,15 @@ rte_eth_bond_slave_remove(uint8_t bonded_port_id, uint8_t slave_port_id) > if (valid_slave_port_id(slave_port_id) != 0) > goto err_del; > > - internals = rte_eth_devices[bonded_port_id].data->dev_private; > + eth_dev = &rte_eth_devices[bonded_port_id]; > + internals = eth_dev->data->dev_private; > > /* first remove from active slave list */ > - for (i = 0; i < internals->active_slave_count; i++) { > - if (internals->active_slaves[i] == slave_port_id) > - pos = i; > - > - /* shift active slaves up active array list */ > - if (pos >= 0 && i < (internals->active_slave_count - 1)) > - internals->active_slaves[i] = internals->active_slaves[i+1]; > - } > + pos = find_slave_by_id(internals->active_slaves, internals->active_slave_count, > + slave_port_id); > > - if (pos >= 0) > - internals->active_slave_count--; > + if (pos < internals->active_slave_count) > + rte_eth_bond_deactive_slave(eth_dev, pos); > > pos = -1; > /* now remove from slave list */ > diff --git a/lib/librte_pmd_bond/rte_eth_bond_pmd.c b/lib/librte_pmd_bond/rte_eth_bond_pmd.c > index 38cc1ae..482ddb8 100644 > --- a/lib/librte_pmd_bond/rte_eth_bond_pmd.c > +++ b/lib/librte_pmd_bond/rte_eth_bond_pmd.c > @@ -447,6 +447,27 @@ link_properties_valid(struct rte_eth_link *bonded_dev_link, > } > > int > +mac_address_get(struct rte_eth_dev *eth_dev, struct ether_addr *dst_mac_addr) > +{ > + struct ether_addr *mac_addr; > + > + mac_addr = eth_dev->data->mac_addrs; > + > + if (eth_dev == NULL) { > + RTE_LOG(ERR, PMD, "%s: NULL pointer eth_dev specified\n", __func__); > + return -1; > + } > + > + if (dst_mac_addr == NULL) { > + RTE_LOG(ERR, PMD, "%s: NULL pointer MAC specified\n", __func__); > + return -1; > + } > + > + ether_addr_copy(mac_addr, dst_mac_addr); > + return 0; > +} > + > +int > mac_address_set(struct rte_eth_dev *eth_dev, struct ether_addr *new_mac_addr) > { > struct ether_addr *mac_addr; > @@ -817,7 +838,7 @@ bond_ethdev_tx_queue_setup(struct rte_eth_dev *dev, uint16_t tx_queue_id, > 0, dev->pci_dev->numa_node); > > if (bd_tx_q == NULL) > - return -1; > + return -1; > > bd_tx_q->queue_id = tx_queue_id; > bd_tx_q->dev_private = dev->data->dev_private; > @@ -940,7 +961,6 @@ bond_ethdev_promiscuous_enable(struct rte_eth_dev *eth_dev) > case BONDING_MODE_ACTIVE_BACKUP: > default: > rte_eth_promiscuous_enable(internals->current_primary_port); > - > } > } > > @@ -975,7 +995,8 @@ bond_ethdev_lsc_event_callback(uint8_t port_id, enum rte_eth_event_type type, > struct bond_dev_private *internals; > struct rte_eth_link link; > > - int i, valid_slave = 0, active_pos = -1; > + int i, valid_slave = 0; > + uint8_t active_pos; > uint8_t lsc_flag = 0; > > if (type != RTE_ETH_EVENT_INTR_LSC || param == NULL) > @@ -1005,16 +1026,12 @@ bond_ethdev_lsc_event_callback(uint8_t port_id, enum rte_eth_event_type type, > return; > > /* Search for port in active port list */ > - for (i = 0; i < internals->active_slave_count; i++) { > - if (port_id == internals->active_slaves[i]) { > - active_pos = i; > - break; > - } > - } > + active_pos = find_slave_by_id(internals->active_slaves, > + internals->active_slave_count, port_id); > > rte_eth_link_get_nowait(port_id, &link); > if (link.link_status) { > - if (active_pos >= 0) > + if (active_pos < internals->active_slave_count) > return; > > /* if no active slave ports then set this port to be primary port */ > @@ -1028,21 +1045,19 @@ bond_ethdev_lsc_event_callback(uint8_t port_id, enum rte_eth_event_type type, > link_properties_set(bonded_eth_dev, > &(slave_eth_dev->data->dev_link)); > } > - internals->active_slaves[internals->active_slave_count++] = port_id; > + > + rte_eth_bond_activate_slave(bonded_eth_dev, port_id); > > /* If user has defined the primary port then default to using it */ > if (internals->user_defined_primary_port && > internals->primary_port == port_id) > bond_ethdev_primary_set(internals, port_id); > } else { > - if (active_pos < 0) > + if (active_pos == internals->active_slave_count) > return; > > /* Remove from active slave list */ > - for (i = active_pos; i < (internals->active_slave_count - 1); i++) > - internals->active_slaves[i] = internals->active_slaves[i+1]; > - > - internals->active_slave_count--; > + rte_eth_bond_deactive_slave(bonded_eth_dev, active_pos); > > /* No active slaves, change link status to down and reset other > * link properties */ > diff --git a/lib/librte_pmd_bond/rte_eth_bond_private.h b/lib/librte_pmd_bond/rte_eth_bond_private.h > index 60f1e8d..6742a4c 100644 > --- a/lib/librte_pmd_bond/rte_eth_bond_private.h > +++ b/lib/librte_pmd_bond/rte_eth_bond_private.h > @@ -115,11 +115,11 @@ struct bond_dev_private { > uint16_t nb_rx_queues; /**< Total number of rx queues */ > uint16_t nb_tx_queues; /**< Total number of tx queues*/ > > - uint8_t slave_count; /**< Number of active slaves */ > - uint8_t active_slave_count; /**< Number of slaves */ > + uint8_t slave_count; /**< Number of slaves */ > + uint8_t active_slave_count; /**< Number of active slaves */ > > - uint8_t active_slaves[RTE_MAX_ETHPORTS]; /**< Active slave list */ > uint8_t slaves[RTE_MAX_ETHPORTS]; /**< Slave list */ > + uint8_t active_slaves[RTE_MAX_ETHPORTS]; /**< Active slave list */ > > /** Persisted configuration of slaves */ > struct slave_conf presisted_slaves_conf[RTE_MAX_ETHPORTS]; > @@ -130,6 +130,19 @@ extern struct eth_dev_ops default_dev_ops; > int > valid_bonded_ethdev(struct rte_eth_dev *eth_dev); > > +static inline uint8_t > +find_slave_by_id(uint8_t *slaves, uint8_t slaves_count, > + uint8_t slave_id ) { > + > + uint8_t pos; > + for (pos = 0; pos < slaves_count; pos++) { > + if (slave_id == slaves[pos]) > + break; > + } > + > + return pos; > +} > + > int > valid_port_id(uint8_t port_id); > > @@ -140,6 +153,14 @@ int > valid_slave_port_id(uint8_t port_id); > > void > +rte_eth_bond_deactive_slave(struct rte_eth_dev *eth_dev, > + uint8_t slave_pos ); > + > +void > +rte_eth_bond_activate_slave(struct rte_eth_dev *eth_dev, > + uint8_t port_id ); > + > +void > link_properties_set(struct rte_eth_dev *bonded_eth_dev, > struct rte_eth_link *slave_dev_link); > void > @@ -153,6 +174,9 @@ int > mac_address_set(struct rte_eth_dev *eth_dev, struct ether_addr *new_mac_addr); > > int > +mac_address_get(struct rte_eth_dev *eth_dev, struct ether_addr *dst_mac_addr); > + > +int > mac_address_slaves_update(struct rte_eth_dev *bonded_eth_dev); > > uint8_t > -- > 1.7.9.5 > >