Re: [dpdk-dev] [PATCH v5 1/3] ixgbe: Cleanups

[Vlad Zolotarov <vladz@cloudius-systems.com>]

On 03/09/15 12:49, Ananyev, Konstantin wrote:
>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Vlad Zolotarov
>> Sent: Monday, March 09, 2015 10:21 AM
>> To: dev@dpdk.org
>> Subject: [dpdk-dev] [PATCH v5 1/3] ixgbe: Cleanups
>>
>>     - Removed the not needed casting.
>>     - ixgbe_dev_rx_init(): shorten the lines by defining a local alias variable to access
>>                            &dev->data->dev_conf.rxmode.
>>
>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>> ---
>>   lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 27 ++++++++++++---------------
>>   1 file changed, 12 insertions(+), 15 deletions(-)
>>
>> diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
>> index 72c65df..609b5fd 100644
>> --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
>> +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
>> @@ -1032,8 +1032,7 @@ ixgbe_rx_alloc_bufs(struct igb_rx_queue *rxq)
>>   	int diag, i;
>>
>>   	/* allocate buffers in bulk directly into the S/W ring */
>> -	alloc_idx = (uint16_t)(rxq->rx_free_trigger -
>> -				(rxq->rx_free_thresh - 1));
>> +	alloc_idx = rxq->rx_free_trigger - (rxq->rx_free_thresh - 1);
> I think all these extra casts came in to keep icc 12.* compiling without warnings.
> I am agree that they are unnecessary.
> Though if we still have to support icc 12.* we either need to keep them, or find
> some other way to keep it happy.
Fix icc maybe?
I'm sorry, but what do I miss here? Both alloc_idx, rxq->rx_free_trigger 
and rxq->rx_free_thresh are uint16_t

So, according to C standard the result is also uint16_t thus no casting 
is needed:

the result of a subtraction generating a negative number in an unsigned type is well-defined:

     1. [...] A computation involving unsigned operands can never
        overflow, because a result that cannot be represented by the
        resulting unsigned integer type is reduced modulo the number
        that is one greater than the largest value that can be
        represented by the resulting type. (ISO/IEC 9899:1999 (E) §6.2.5/9)


Could u, pls., be more specific and send here the error generated by icc 
after my patches?

thanks,
vlad



> Konstantin
>
>>   	rxep = &rxq->sw_ring[alloc_idx];
>>   	diag = rte_mempool_get_bulk(rxq->mb_pool, (void *)rxep,
>>   				    rxq->rx_free_thresh);
>> @@ -1061,10 +1060,9 @@ ixgbe_rx_alloc_bufs(struct igb_rx_queue *rxq)
>>   	IXGBE_PCI_REG_WRITE(rxq->rdt_reg_addr, rxq->rx_free_trigger);
>>
>>   	/* update state of internal queue structure */
>> -	rxq->rx_free_trigger = (uint16_t)(rxq->rx_free_trigger +
>> -						rxq->rx_free_thresh);
>> +	rxq->rx_free_trigger = rxq->rx_free_trigger + rxq->rx_free_thresh;
>>   	if (rxq->rx_free_trigger >= rxq->nb_rx_desc)
>> -		rxq->rx_free_trigger = (uint16_t)(rxq->rx_free_thresh - 1);
>> +		rxq->rx_free_trigger = rxq->rx_free_thresh - 1;
>>
>>   	/* no errors */
>>   	return 0;
>> @@ -3560,6 +3558,7 @@ ixgbe_dev_rx_init(struct rte_eth_dev *dev)
>>   	uint32_t rxcsum;
>>   	uint16_t buf_size;
>>   	uint16_t i;
>> +	struct rte_eth_rxmode *rx_conf = &dev->data->dev_conf.rxmode;
>>
>>   	PMD_INIT_FUNC_TRACE();
>>   	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>> @@ -3582,7 +3581,7 @@ ixgbe_dev_rx_init(struct rte_eth_dev *dev)
>>   	 * Configure CRC stripping, if any.
>>   	 */
>>   	hlreg0 = IXGBE_READ_REG(hw, IXGBE_HLREG0);
>> -	if (dev->data->dev_conf.rxmode.hw_strip_crc)
>> +	if (rx_conf->hw_strip_crc)
>>   		hlreg0 |= IXGBE_HLREG0_RXCRCSTRP;
>>   	else
>>   		hlreg0 &= ~IXGBE_HLREG0_RXCRCSTRP;
>> @@ -3590,11 +3589,11 @@ ixgbe_dev_rx_init(struct rte_eth_dev *dev)
>>   	/*
>>   	 * Configure jumbo frame support, if any.
>>   	 */
>> -	if (dev->data->dev_conf.rxmode.jumbo_frame == 1) {
>> +	if (rx_conf->jumbo_frame == 1) {
>>   		hlreg0 |= IXGBE_HLREG0_JUMBOEN;
>>   		maxfrs = IXGBE_READ_REG(hw, IXGBE_MAXFRS);
>>   		maxfrs &= 0x0000FFFF;
>> -		maxfrs |= (dev->data->dev_conf.rxmode.max_rx_pkt_len << 16);
>> +		maxfrs |= (rx_conf->max_rx_pkt_len << 16);
>>   		IXGBE_WRITE_REG(hw, IXGBE_MAXFRS, maxfrs);
>>   	} else
>>   		hlreg0 &= ~IXGBE_HLREG0_JUMBOEN;
>> @@ -3618,9 +3617,7 @@ ixgbe_dev_rx_init(struct rte_eth_dev *dev)
>>   		 * Reset crc_len in case it was changed after queue setup by a
>>   		 * call to configure.
>>   		 */
>> -		rxq->crc_len = (uint8_t)
>> -				((dev->data->dev_conf.rxmode.hw_strip_crc) ? 0 :
>> -				ETHER_CRC_LEN);
>> +		rxq->crc_len = rx_conf->hw_strip_crc ? 0 : ETHER_CRC_LEN;
>>
>>   		/* Setup the Base and Length of the Rx Descriptor Rings */
>>   		bus_addr = rxq->rx_ring_phys_addr;
>> @@ -3638,7 +3635,7 @@ ixgbe_dev_rx_init(struct rte_eth_dev *dev)
>>   		/*
>>   		 * Configure Header Split
>>   		 */
>> -		if (dev->data->dev_conf.rxmode.header_split) {
>> +		if (rx_conf->header_split) {
>>   			if (hw->mac.type == ixgbe_mac_82599EB) {
>>   				/* Must setup the PSRTYPE register */
>>   				uint32_t psrtype;
>> @@ -3648,7 +3645,7 @@ ixgbe_dev_rx_init(struct rte_eth_dev *dev)
>>   					IXGBE_PSRTYPE_IPV6HDR;
>>   				IXGBE_WRITE_REG(hw, IXGBE_PSRTYPE(rxq->reg_idx), psrtype);
>>   			}
>> -			srrctl = ((dev->data->dev_conf.rxmode.split_hdr_size <<
>> +			srrctl = ((rx_conf->split_hdr_size <<
>>   				IXGBE_SRRCTL_BSIZEHDRSIZE_SHIFT) &
>>   				IXGBE_SRRCTL_BSIZEHDR_MASK);
>>   			srrctl |= IXGBE_SRRCTL_DESCTYPE_HDR_SPLIT_ALWAYS;
>> @@ -3699,7 +3696,7 @@ ixgbe_dev_rx_init(struct rte_eth_dev *dev)
>>   	 */
>>   	rxcsum = IXGBE_READ_REG(hw, IXGBE_RXCSUM);
>>   	rxcsum |= IXGBE_RXCSUM_PCSD;
>> -	if (dev->data->dev_conf.rxmode.hw_ip_checksum)
>> +	if (rx_conf->hw_ip_checksum)
>>   		rxcsum |= IXGBE_RXCSUM_IPPCSE;
>>   	else
>>   		rxcsum &= ~IXGBE_RXCSUM_IPPCSE;
>> @@ -3709,7 +3706,7 @@ ixgbe_dev_rx_init(struct rte_eth_dev *dev)
>>   	if (hw->mac.type == ixgbe_mac_82599EB ||
>>   	    hw->mac.type == ixgbe_mac_X540) {
>>   		rdrxctl = IXGBE_READ_REG(hw, IXGBE_RDRXCTL);
>> -		if (dev->data->dev_conf.rxmode.hw_strip_crc)
>> +		if (rx_conf->hw_strip_crc)
>>   			rdrxctl |= IXGBE_RDRXCTL_CRCSTRIP;
>>   		else
>>   			rdrxctl &= ~IXGBE_RDRXCTL_CRCSTRIP;
>> --
>> 2.1.0