netdev - RE: [net-next 5/5] ixgbe: convert to ndo_fix

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Tue, 12 Jul 2011 15:11:52 -0700
From:	"Skidmore, Donald C" <donald.c.skidmore@...el.com>
To:	Michal Miroslaw <mirq-linux@...e.qmqm.pl>
CC:	"Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>,
	"davem@...emloft.net" <davem@...emloft.net>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"gospo@...hat.com" <gospo@...hat.com>
Subject: RE: [net-next 5/5] ixgbe: convert to ndo_fix_features



>-----Original Message-----
>From: Michal Miroslaw [mailto:mirq-linux@...e.qmqm.pl]
>Sent: Tuesday, July 12, 2011 2:45 PM
>To: Skidmore, Donald C
>Cc: Kirsher, Jeffrey T; davem@...emloft.net; netdev@...r.kernel.org;
>gospo@...hat.com
>Subject: Re: [net-next 5/5] ixgbe: convert to ndo_fix_features
>
>Good to see the idea converging. :) More hints below.
>
>On Tue, Jul 12, 2011 at 02:28:15PM -0700, Skidmore, Donald C wrote:
>[...]
>> The logic in this conditional is a little complex. I believe it would
>> end up looking something like the following:
>>
>> in fix_features:
>>
>> 	/* Prevent change if not RSC capable or invalid ITR setting */
>> 	if (data & NETIF_F_LRO)
>
>That's not a fast path - you can drop (data & NETIF_F_LRO) check.

Nice - I was spending too much time looking at the old code didn't notice this.

>
>> 		if (!(adapter->flags2 & IXGBE_FLAG2_RSC_CAPABLE) {
>> 			data &= ~NETIF_F_LRO;
>> 		else if (!(adapter->flags2 & IXGBE_FLAG2_RSC_ENABLED) &&
>> 	    		   (adapter->rx_itr_setting != 1 &&
>> 			    adapter->rx_itr_setting > IXGBE_MAX_RSC_INT_RATE))
>{
>> 				data &= ~NETIF_F_LRO;
>> 		}
>> 	}
>
>Same for !(adapter->flags2 & IXGBE_FLAG2_RSC_ENABLED). After this
>change,
>you might add a call to netdev_update_features() whenever the second
>part
>of the condition is changed by user and you get correctness all the time
>'for free'.

I like this too and makes much more sense to me.  This was a reflection of the original code but really didn't matter since anytime RSC_ENABLED was set the ITR values would have been correct anyway.
   
>
>> in set_feature:
>>
>> 	/* Make sure RSC matches LRO, reset if change */
>> 	if (!!(data & NETIF_F_LRO) !=
>> 	    !!(adapter->flags2 & IXGBE_FLAG2_RSC_ENABLED)) {
>> 		adapter->flags2 ^= IXGBE_FLAG2_RSC_ENABLED;
>> 		switch (adapter->hw.mac.type) {
>> 		case ixgbe_mac_X540:
>> 		case ixgbe_mac_82599EB:
>> 			need_reset = true;
>> 			break;
>> 		default:
>> 			break;
>> 		}
>> 	}
>
>Correct. Why would you need IXGBE_FLAG2_RSC_ENABLED now, when it just
>mirrors NETIF_F_LRO?
>

We currently have a lot of checks for the IXGBE_FLAG2_RSC_ENABLED flag all over the code.  I bet your next question is why not just using NETIF_F_LRO. :)  Well this same code base is used in our source forge driver that has to be backward compatible with older kernels.  It might be possible to do the kcompat magic to make it all work out but it won't be trivial and I didn't want to include it in this patch.  I had planned on looking at it in the future to see if it was possible/worth doing. 

>[...]
>> Thanks again for the clarifications, the netdev-features.txt helped a
>lot too.
>
>Nice to hear (read) that!
>
>Best Regards,
>Michał Mirosław

I'll try to get a patch out soon.

Thanks again,
-Don Skidmore <donald.c.skidmore@...el.com>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html