| Message ID | 1386382643-29055-10-git-send-email-jeffrey.t.kirsher@intel.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
Hello. On 07-12-2013 6:17, Jeff Kirsher wrote: > From: Anjali Singhai Jain <anjali.singhai@intel.com> > RSS can steer packets based on recognition of all > sorts of different headers. Enable some more of them. > Change-Id: I2264dedae66fb0bceca6fb6e772e050e3ca8efc8 This line has no place in the upstream patches, and I'm seeing it in several patches of this series. > Signed-off-by: Anjali Singhai Jain <anjali.singhai@intel.com> > Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com> > Tested-by: Kavindya Deegala <kavindya.s.deegala@intel.com> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com> > --- > drivers/net/ethernet/intel/i40e/i40e_main.c | 38 ++++++++++++++++++----------- > 1 file changed, 24 insertions(+), 14 deletions(-) > diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c > index 273db99..40c64c4 100644 > --- a/drivers/net/ethernet/intel/i40e/i40e_main.c > +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c > @@ -5615,15 +5615,34 @@ static int i40e_setup_misc_vector(struct i40e_pf *pf) > **/ > static int i40e_config_rss(struct i40e_pf *pf) > { > - struct i40e_hw *hw = &pf->hw; > - u32 lut = 0; > - int i, j; > - u64 hena; > + const u64 default_hena = > + ((u64)1 << I40E_FILTER_PCTYPE_NONF_UNICAST_IPV4_UDP) | 1ULL not good enough? [...] WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 2013-12-07 at 22:00 +0400, Sergei Shtylyov wrote: > On 07-12-2013 6:17, Jeff Kirsher wrote: > > > From: Anjali Singhai Jain <anjali.singhai@intel.com> > > > RSS can steer packets based on recognition of all > > sorts of different headers. Enable some more of them. > > > Change-Id: I2264dedae66fb0bceca6fb6e772e050e3ca8efc8 > > This line has no place in the upstream patches, and I'm seeing it > in > several patches of this series. This is an internal hash id, so that we can track upstream changes back to internal git tree changes. I did verify that this was acceptable and is being used in the kernel in upstream patches before pushing this information. > > > Signed-off-by: Anjali Singhai Jain <anjali.singhai@intel.com> > > Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com> > > Tested-by: Kavindya Deegala <kavindya.s.deegala@intel.com> > > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com> > > --- > > drivers/net/ethernet/intel/i40e/i40e_main.c | 38 > ++++++++++++++++++----------- > > 1 file changed, 24 insertions(+), 14 deletions(-)
On Sat, 2013-12-07 at 23:12 +0300, Sergei Shtylyov wrote: > On 12/07/2013 09:16 PM, Jeff Kirsher wrote: > > >>> From: Anjali Singhai Jain <anjali.singhai@intel.com> > > >>> RSS can steer packets based on recognition of all > >>> sorts of different headers. Enable some more of them. > > >>> Change-Id: I2264dedae66fb0bceca6fb6e772e050e3ca8efc8 > > >> This line has no place in the upstream patches, and I'm seeing > it > >> in > >> several patches of this series. > > > This is an internal hash id, so that we can track upstream changes > back > > to internal git tree changes. I did verify that this was acceptable > and > > is being used in the kernel in upstream patches before pushing this > > information. > > First time I hear about that. Its use is constantly denied in > linux-usb at > least. Maybe netdev has its own rules regarding this though... Take a look at `git log --format=oneline --grep="Change-Id"` You will see there are instances in arch as well as in wireless and in other areas as well. So we are not doing something new or the only ones using this tag. Google appears to be using it as well based the following information: https://gerrit-documentation.storage.googleapis.com/Documentation/2.7/user-changeid.html Places using gerrit: http://code.google.com/p/gerrit/wiki/ShowCases This is the largest example of a "Change-Id:" tag that I know of.
Hello. On 12/07/2013 09:16 PM, Jeff Kirsher wrote: >>> From: Anjali Singhai Jain <anjali.singhai@intel.com> >>> RSS can steer packets based on recognition of all >>> sorts of different headers. Enable some more of them. >>> Change-Id: I2264dedae66fb0bceca6fb6e772e050e3ca8efc8 >> This line has no place in the upstream patches, and I'm seeing it >> in >> several patches of this series. > This is an internal hash id, so that we can track upstream changes back > to internal git tree changes. I did verify that this was acceptable and > is being used in the kernel in upstream patches before pushing this > information. First time I hear about that. Its use is constantly denied in linux-usb at least. Maybe netdev has its own rules regarding this though... WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 2013-12-08 at 00:20 +0300, Sergei Shtylyov wrote: > On 12/07/2013 10:36 PM, Jeff Kirsher wrote: > > >>>>> From: Anjali Singhai Jain <anjali.singhai@intel.com> > > >>>>> RSS can steer packets based on recognition of all > >>>>> sorts of different headers. Enable some more of them. > > >>>>> Change-Id: I2264dedae66fb0bceca6fb6e772e050e3ca8efc8 > > >>>> This line has no place in the upstream patches, and I'm seeing it > >>>> in several patches of this series. > > >>> This is an internal hash id, so that we can track upstream changes back > >>> to internal git tree changes. I did verify that this was acceptable and > >>> is being used in the kernel in upstream patches before pushing this > >>> information. > > >> First time I hear about that. Its use is constantly denied in > >> linux-usb at least. Maybe netdev has its own rules regarding this though... > > Greg KH seems to be persistent in being negative towards its use. I also > react semi-automatically whenever I see it, asking to remove it. Anyway, it's > really DaveM's issue whether to allow it. > The main issue with it is that it doesn't contain any useful information > to an ordinary person browsing kernel commits, it's exactly for the internal > use only. Well, I would disagree on the point that it would be for internal use only. While it is a internal git hash, it helps us support the Linux community by helping us track the internal history of the patch for support and changes. So customers could give us either the Linux kernel hash (in which turn we would find the internal hash id information from the patch description) or could give us the I<hash>. So you are right, we would be the ones using the information to help support you and everyone else. Personally, the more information you can put in a patch description (without writing a novel), the better. While I was not originally a fan of the "Change-Id:", the positives out-weighed any negative argument. > > > Take a look at `git log --format=oneline --grep="Change-Id"` > > You will see there are instances in arch as well as in wireless and in > > other areas as well. > > I saw only 3 screens of commits (expected to see more) without much system. > > > So we are not doing something new or the only ones using this tag. > > I didn't say that (in fact, quite the contrary). Sorry, I inferred that from the response you originally sent. > > > Google appears to be using it as well based the following information: > > https://gerrit-documentation.storage.googleapis.com/Documentation/2.7/user-changeid.html > > Why should we care about what Google does? I was just giving you background on where this tag appeared to originate from. I was not trying to say "Ooo Google does this, so we should too..." > > > Places using gerrit: > > http://code.google.com/p/gerrit/wiki/ShowCases > > This is the largest example of a "Change-Id:" tag that I know of. > > I didn't see the Linux kernel in this list, only Android. Neither did I, but I did find the use of it in the Linux kernel before we decided to adopt the practice.
On 12/07/2013 10:36 PM, Jeff Kirsher wrote: >>>>> From: Anjali Singhai Jain <anjali.singhai@intel.com> >>>>> RSS can steer packets based on recognition of all >>>>> sorts of different headers. Enable some more of them. >>>>> Change-Id: I2264dedae66fb0bceca6fb6e772e050e3ca8efc8 >>>> This line has no place in the upstream patches, and I'm seeing it >>>> in several patches of this series. >>> This is an internal hash id, so that we can track upstream changes back >>> to internal git tree changes. I did verify that this was acceptable and >>> is being used in the kernel in upstream patches before pushing this >>> information. >> First time I hear about that. Its use is constantly denied in >> linux-usb at least. Maybe netdev has its own rules regarding this though... Greg KH seems to be persistent in being negative towards its use. I also react semi-automatically whenever I see it, asking to remove it. Anyway, it's really DaveM's issue whether to allow it. The main issue with it is that it doesn't contain any useful information to an ordinary person browsing kernel commits, it's exactly for the internal use only. > Take a look at `git log --format=oneline --grep="Change-Id"` > You will see there are instances in arch as well as in wireless and in > other areas as well. I saw only 3 screens of commits (expected to see more) without much system. > So we are not doing something new or the only ones using this tag. I didn't say that (in fact, quite the contrary). > Google appears to be using it as well based the following information: > https://gerrit-documentation.storage.googleapis.com/Documentation/2.7/user-changeid.html Why should we care about what Google does? > Places using gerrit: > http://code.google.com/p/gerrit/wiki/ShowCases > This is the largest example of a "Change-Id:" tag that I know of. I didn't see the Linux kernel in this list, only Android. WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> Date: Sat, 07 Dec 2013 22:00:21 +0400 > This line has no place in the upstream patches, and I'm seeing it in > several patches of this series. I told Jeff that this was OK, other entities such as Google do it too. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Dec 7, 2013 at 10:55 PM, Jeff Kirsher <jeffrey.t.kirsher@intel.com> wrote: > On Sun, 2013-12-08 at 00:20 +0300, Sergei Shtylyov wrote: >> On 12/07/2013 10:36 PM, Jeff Kirsher wrote: >> >> >>>>> From: Anjali Singhai Jain <anjali.singhai@intel.com> >> >> >>>>> RSS can steer packets based on recognition of all >> >>>>> sorts of different headers. Enable some more of them. >> >> >>>>> Change-Id: I2264dedae66fb0bceca6fb6e772e050e3ca8efc8 >> >> >>>> This line has no place in the upstream patches, and I'm seeing it >> >>>> in several patches of this series. >> >> >>> This is an internal hash id, so that we can track upstream changes back >> >>> to internal git tree changes. I did verify that this was acceptable and >> >>> is being used in the kernel in upstream patches before pushing this >> >>> information. >> >> >> First time I hear about that. Its use is constantly denied in >> >> linux-usb at least. Maybe netdev has its own rules regarding this though... >> >> Greg KH seems to be persistent in being negative towards its use. I also >> react semi-automatically whenever I see it, asking to remove it. Anyway, it's >> really DaveM's issue whether to allow it. >> The main issue with it is that it doesn't contain any useful information >> to an ordinary person browsing kernel commits, it's exactly for the internal >> use only. > > Well, I would disagree on the point that it would be for internal use > only. While it is a internal git hash, it helps us support the Linux > community by helping us track the internal history of the patch for > support and changes. So customers could give us either the Linux kernel > hash (in which turn we would find the internal hash id information from > the patch description) or could give us the I<hash>. So you are right, > we would be the ones using the information to help support you and > everyone else. Jeff, thanks for sharing your thoughts and the deeper reasoning. I am still not fully (and not that it has to be gating factor, but still, lets discuss that a bit...) convinced -- e.g you mention internal git -- but the patch can be gone through large changes throughout the submission process and @ the exterme can be totally different in upstream vs. how it was in the internal git once submitted. Shouldn't vendors actually seek to do that the other way around? e.g find a way to link plain upstream commit Hash to their feature request / bug reports / development trees and not plant the IHash on upstream? just thinking out loud. > Personally, the more information you can put in a patch description > (without writing a novel), the better. While I was not originally a fan > of the "Change-Id:", the positives out-weighed any negative argument. > >> >> > Take a look at `git log --format=oneline --grep="Change-Id"` >> > You will see there are instances in arch as well as in wireless and in >> > other areas as well. >> >> I saw only 3 screens of commits (expected to see more) without much system. >> >> > So we are not doing something new or the only ones using this tag. >> >> I didn't say that (in fact, quite the contrary). > > Sorry, I inferred that from the response you originally sent. > >> >> > Google appears to be using it as well based the following information: >> > https://gerrit-documentation.storage.googleapis.com/Documentation/2.7/user-changeid.html >> >> Why should we care about what Google does? > > I was just giving you background on where this tag appeared to originate > from. I was not trying to say "Ooo Google does this, so we should > too..." > >> >> > Places using gerrit: >> > http://code.google.com/p/gerrit/wiki/ShowCases >> > This is the largest example of a "Change-Id:" tag that I know of. >> >> I didn't see the Linux kernel in this list, only Android. > > Neither did I, but I did find the use of it in the Linux kernel before > we decided to adopt the practice. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 10, 2013 at 12:05 AM, Or Gerlitz <or.gerlitz@gmail.com> wrote: > On Sat, Dec 7, 2013 at 10:55 PM, Jeff Kirsher <jeffrey.t.kirsher@intel.com> wrote: >> On Sun, 2013-12-08 at 00:20 +0300, Sergei Shtylyov wrote: >>> Greg KH seems to be persistent in being negative towards its use. I also >>> react semi-automatically whenever I see it, asking to remove it. Anyway, it's >>> really DaveM's issue whether to allow it. >>> The main issue with it is that it doesn't contain any useful information >>> to an ordinary person browsing kernel commits, it's exactly for the internal use only. >> Well, I would disagree on the point that it would be for internal use >> only. While it is a internal git hash, it helps us support the Linux >> community by helping us track the internal history of the patch for >> support and changes. So customers could give us either the Linux kernel >> hash (in which turn we would find the internal hash id information from >> the patch description) or could give us the I<hash>. So you are right, >> we would be the ones using the information to help support you and >> everyone else. > Shouldn't vendors actually seek to do that the other way around? e.g > find a way to link plain upstream commit Hash to their feature request > / bug reports / development trees and not plant the IHash on upstream? just thinking out loud. e.g what's wrong with having the maintainer of the internal git tree do "git reword" for the internal commit after it has been accepted into upstream and adding there the upstream hash. This will create a link with the Internal Change-Id: w.o putting vendors' change-Id: on upstream commits, thoughts? Or. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c index 273db99..40c64c4 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_main.c +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c @@ -5615,15 +5615,34 @@ static int i40e_setup_misc_vector(struct i40e_pf *pf) **/ static int i40e_config_rss(struct i40e_pf *pf) { - struct i40e_hw *hw = &pf->hw; - u32 lut = 0; - int i, j; - u64 hena; + const u64 default_hena = + ((u64)1 << I40E_FILTER_PCTYPE_NONF_UNICAST_IPV4_UDP) | + ((u64)1 << I40E_FILTER_PCTYPE_NONF_MULTICAST_IPV4_UDP) | + ((u64)1 << I40E_FILTER_PCTYPE_NONF_IPV4_UDP) | + ((u64)1 << I40E_FILTER_PCTYPE_NONF_IPV4_SCTP) | + ((u64)1 << I40E_FILTER_PCTYPE_NONF_IPV4_TCP_SYN) | + ((u64)1 << I40E_FILTER_PCTYPE_NONF_IPV4_TCP) | + ((u64)1 << I40E_FILTER_PCTYPE_NONF_IPV4_OTHER) | + ((u64)1 << I40E_FILTER_PCTYPE_FRAG_IPV4) | + ((u64)1 << I40E_FILTER_PCTYPE_NONF_UNICAST_IPV6_UDP) | + ((u64)1 << I40E_FILTER_PCTYPE_NONF_MULTICAST_IPV6_UDP) | + ((u64)1 << I40E_FILTER_PCTYPE_NONF_IPV6_UDP) | + ((u64)1 << I40E_FILTER_PCTYPE_NONF_IPV6_TCP_SYN) | + ((u64)1 << I40E_FILTER_PCTYPE_NONF_IPV6_TCP) | + ((u64)1 << I40E_FILTER_PCTYPE_NONF_IPV6_SCTP) | + ((u64)1 << I40E_FILTER_PCTYPE_NONF_IPV6_OTHER) | + ((u64)1 << I40E_FILTER_PCTYPE_FRAG_IPV6) | + ((u64)1 << I40E_FILTER_PCTYPE_L2_PAYLOAD); + /* Set of random keys generated using kernel random number generator */ static const u32 seed[I40E_PFQF_HKEY_MAX_INDEX + 1] = {0x41b01687, 0x183cfd8c, 0xce880440, 0x580cbc3c, 0x35897377, 0x328b25e1, 0x4fa98922, 0xb7d90c14, 0xd5bad70d, 0xcd15a2c1, 0xe8580225, 0x4a1e9d11, 0xfe5731be}; + struct i40e_hw *hw = &pf->hw; + u32 lut = 0; + int i, j; + u64 hena; /* Fill out hash function seed */ for (i = 0; i <= I40E_PFQF_HKEY_MAX_INDEX; i++) @@ -5632,16 +5651,7 @@ static int i40e_config_rss(struct i40e_pf *pf) /* By default we enable TCP/UDP with IPv4/IPv6 ptypes */ hena = (u64)rd32(hw, I40E_PFQF_HENA(0)) | ((u64)rd32(hw, I40E_PFQF_HENA(1)) << 32); - hena |= ((u64)1 << I40E_FILTER_PCTYPE_NONF_IPV4_UDP) | - ((u64)1 << I40E_FILTER_PCTYPE_NONF_UNICAST_IPV4_UDP) | - ((u64)1 << I40E_FILTER_PCTYPE_NONF_MULTICAST_IPV4_UDP) | - ((u64)1 << I40E_FILTER_PCTYPE_NONF_IPV4_TCP) | - ((u64)1 << I40E_FILTER_PCTYPE_NONF_IPV6_TCP) | - ((u64)1 << I40E_FILTER_PCTYPE_NONF_IPV6_UDP) | - ((u64)1 << I40E_FILTER_PCTYPE_NONF_UNICAST_IPV6_UDP) | - ((u64)1 << I40E_FILTER_PCTYPE_NONF_MULTICAST_IPV6_UDP) | - ((u64)1 << I40E_FILTER_PCTYPE_FRAG_IPV4)| - ((u64)1 << I40E_FILTER_PCTYPE_FRAG_IPV6); + hena |= default_hena; wr32(hw, I40E_PFQF_HENA(0), (u32)hena); wr32(hw, I40E_PFQF_HENA(1), (u32)(hena >> 32));