| Message ID | 1389271944-26227-6-git-send-email-jeffrey.t.kirsher@intel.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
Hello. On 09-01-2014 16:52, Jeff Kirsher wrote: > From: Greg Rose <gregory.v.rose@intel.com> > The memory barrier used in maybe_stop_tx can use a comment. > Also add checks to VSI->rx_rings to ensure a kernel panic is not induced. Don't see why this is made in one patch instead of two since the two things look completely unrelated and even modifying different files. > Change-ID: I48cc1bf1d6cf301818155b737edeef77c0d790c7 > Change-ID: I1363a8445fbf521a26267849966296ed55f43ad8 Why even 2 of them? > Signed-off-by: Greg Rose <gregory.v.rose@intel.com> > Signed-off-by: Mitch Williams <mitch.a.williams@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> 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 Thu, 9 Jan 2014 18:42:00 +0400 Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> wrote: > Hello. > > On 09-01-2014 16:52, Jeff Kirsher wrote: > > > From: Greg Rose <gregory.v.rose@intel.com> > > > The memory barrier used in maybe_stop_tx can use a comment. > > > Also add checks to VSI->rx_rings to ensure a kernel panic is not > > induced. > > Don't see why this is made in one patch instead of two since the > two things look completely unrelated and even modifying different > files. > > > Change-ID: I48cc1bf1d6cf301818155b737edeef77c0d790c7 > > Change-ID: I1363a8445fbf521a26267849966296ed55f43ad8 > > Why even 2 of them? > > > Signed-off-by: Greg Rose <gregory.v.rose@intel.com> > > Signed-off-by: Mitch Williams <mitch.a.williams@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> > > WBR, Sergei > Yes, it's apparent that two different internal patches were compressed together. If its unacceptable to do this then I'll speak to Jeff about splitting them. - Greg -- 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: Greg Rose <gregory.v.rose@intel.com> Date: Thu, 9 Jan 2014 09:17:35 -0800 > Yes, it's apparent that two different internal patches were compressed > together. If its unacceptable to do this then I'll speak to Jeff about > splitting them. I won't reject the entire pull request on account of this, but please don't do this in the future. -- 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
> -----Original Message----- > From: David Miller [mailto:davem@davemloft.net] > Sent: Thursday, January 09, 2014 12:12 PM > To: Rose, Gregory V > Cc: sergei.shtylyov@cogentembedded.com; Kirsher, Jeffrey T; > netdev@vger.kernel.org; gospo@redhat.com; sassmann@redhat.com; Williams, > Mitch A; Brandeburg, Jesse > Subject: Re: [net-next 05/15] i40e: add a comment on barrier and fix panic > on reset > > From: Greg Rose <gregory.v.rose@intel.com> > Date: Thu, 9 Jan 2014 09:17:35 -0800 > > > Yes, it's apparent that two different internal patches were compressed > > together. If its unacceptable to do this then I'll speak to Jeff > > about splitting them. > > I won't reject the entire pull request on account of this, but please > don't do this in the future. Thanks Dave, We've discussed this internally and ahem... mistakes were made. We'll look out for it in the future. - Greg -- 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 Thu, 2014-01-09 at 15:12 -0500, David Miller wrote: > From: Greg Rose <gregory.v.rose@intel.com> > Date: Thu, 9 Jan 2014 09:17:35 -0800 > > > Yes, it's apparent that two different internal patches were compressed > > together. If its unacceptable to do this then I'll speak to Jeff about > > splitting them. > > I won't reject the entire pull request on account of this, but please > don't do this in the future. I don't understand this. You don't want to see patches that are known to introduce regressions, and you expect people to squash together known buggy patches with their subsequent fix-ups before submitting. So, if a Change-Id is supposed to refer back to an 'original' version of a patch/commit, shouldn't there sometimes be more than one of them in the version that goes upstream? You do something very similar yourself sometimes, with upstream references in stable backports. Ben.
On Thu, 2014-01-09 at 21:21 +0000, Ben Hutchings wrote: > On Thu, 2014-01-09 at 15:12 -0500, David Miller wrote: > > From: Greg Rose <gregory.v.rose@intel.com> > > Date: Thu, 9 Jan 2014 09:17:35 -0800 > > > > > Yes, it's apparent that two different internal patches were compressed > > > together. If its unacceptable to do this then I'll speak to Jeff about > > > splitting them. > > > > I won't reject the entire pull request on account of this, but please > > don't do this in the future. > > I don't understand this. You don't want to see patches that are known > to introduce regressions, and you expect people to squash together known > buggy patches with their subsequent fix-ups before submitting. [...] Hmm, looking at this again, I suppose it is just this particular combination of two unrelated changes you're objecting to? Ben.
From: Ben Hutchings <bhutchings@solarflare.com> Date: Thu, 9 Jan 2014 21:23:10 +0000 > On Thu, 2014-01-09 at 21:21 +0000, Ben Hutchings wrote: >> On Thu, 2014-01-09 at 15:12 -0500, David Miller wrote: >> > From: Greg Rose <gregory.v.rose@intel.com> >> > Date: Thu, 9 Jan 2014 09:17:35 -0800 >> > >> > > Yes, it's apparent that two different internal patches were compressed >> > > together. If its unacceptable to do this then I'll speak to Jeff about >> > > splitting them. >> > >> > I won't reject the entire pull request on account of this, but please >> > don't do this in the future. >> >> I don't understand this. You don't want to see patches that are known >> to introduce regressions, and you expect people to squash together known >> buggy patches with their subsequent fix-ups before submitting. > [...] > > Hmm, looking at this again, I suppose it is just this particular > combination of two unrelated changes you're objecting to? Yes. -- 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 Jan 9, 2014, at 4:52 AM, Jeff Kirsher <jeffrey.t.kirsher@intel.com> wrote: > From: Greg Rose <gregory.v.rose@intel.com> > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c > index ea76134..5cdc67c 100644 > --- a/drivers/net/ethernet/intel/i40e/i40e_main.c > +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c > @@ -422,7 +422,7 @@ void i40e_vsi_reset_stats(struct i40e_vsi *vsi) > memset(&vsi->net_stats_offsets, 0, sizeof(vsi->net_stats_offsets)); > memset(&vsi->eth_stats, 0, sizeof(vsi->eth_stats)); > memset(&vsi->eth_stats_offsets, 0, sizeof(vsi->eth_stats_offsets)); > - if (vsi->rx_rings) > + if (vsi->rx_rings && vsi->rx_rings[0]) { Any reason why just [0] is checked for !NULL here... > for (i = 0; i < vsi->num_queue_pairs; i++) { > memset(&vsi->rx_rings[i]->stats, 0 , > sizeof(vsi->rx_rings[i]->stats)); > @@ -433,6 +433,7 @@ void i40e_vsi_reset_stats(struct i40e_vsi *vsi) > memset(&vsi->tx_rings[i]->tx_stats, 0, > sizeof(vsi->tx_rings[i]->tx_stats)); > } > + } > vsi->stat_offsets_loaded = false; > } > > @@ -2101,8 +2105,11 @@ static void i40e_vsi_free_rx_resources(struct i40e_vsi *vsi) > { > int i; > > + if (!vsi->rx_rings) > + return; > + > for (i = 0; i < vsi->num_queue_pairs; i++) > - if (vsi->rx_rings[i]->desc) > + if (vsi->rx_rings[i] && vsi->rx_rings[i]->desc) but here every [i] is checked for !NULL here? > i40e_free_rx_resources(vsi->rx_rings[i]); > } > If [0] check is sufficient to know if array members are allocated, maybe an wrapper func would help document intent: static bool i40e_vsi_rings_allocated(struct i40e_ring *ring) { return (ring && ring[0]); } -scott -- 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
> -----Original Message----- > From: Scott Feldman [mailto:sfeldma@cumulusnetworks.com] > Sent: Thursday, January 09, 2014 2:17 PM > To: Kirsher, Jeffrey T > Cc: David Miller; Rose, Gregory V; Netdev; gospo@redhat.com; > sassmann@redhat.com; Williams, Mitch A; Brandeburg, Jesse > Subject: Re: [net-next 05/15] i40e: add a comment on barrier and fix panic > on reset > > > On Jan 9, 2014, at 4:52 AM, Jeff Kirsher <jeffrey.t.kirsher@intel.com> > wrote: > > > From: Greg Rose <gregory.v.rose@intel.com> > > > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c > b/drivers/net/ethernet/intel/i40e/i40e_main.c > > index ea76134..5cdc67c 100644 > > --- a/drivers/net/ethernet/intel/i40e/i40e_main.c > > +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c > > @@ -422,7 +422,7 @@ void i40e_vsi_reset_stats(struct i40e_vsi *vsi) > > memset(&vsi->net_stats_offsets, 0, sizeof(vsi->net_stats_offsets)); > > memset(&vsi->eth_stats, 0, sizeof(vsi->eth_stats)); > > memset(&vsi->eth_stats_offsets, 0, sizeof(vsi->eth_stats_offsets)); > > - if (vsi->rx_rings) > > + if (vsi->rx_rings && vsi->rx_rings[0]) { > > Any reason why just [0] is checked for !NULL here... > > > for (i = 0; i < vsi->num_queue_pairs; i++) { > > memset(&vsi->rx_rings[i]->stats, 0 , > > sizeof(vsi->rx_rings[i]->stats)); > > @@ -433,6 +433,7 @@ void i40e_vsi_reset_stats(struct i40e_vsi *vsi) > > memset(&vsi->tx_rings[i]->tx_stats, 0, > > sizeof(vsi->tx_rings[i]->tx_stats)); > > } > > + } > > vsi->stat_offsets_loaded = false; > > } > > > > @@ -2101,8 +2105,11 @@ static void i40e_vsi_free_rx_resources(struct > i40e_vsi *vsi) > > { > > int i; > > > > + if (!vsi->rx_rings) > > + return; > > + > > for (i = 0; i < vsi->num_queue_pairs; i++) > > - if (vsi->rx_rings[i]->desc) > > + if (vsi->rx_rings[i] && vsi->rx_rings[i]->desc) > > but here every [i] is checked for !NULL here? > > > i40e_free_rx_resources(vsi->rx_rings[i]); > > } > > > > If [0] check is sufficient to know if array members are allocated, maybe an > wrapper func would help document intent: > > static bool i40e_vsi_rings_allocated(struct i40e_ring *ring) > { > return (ring && ring[0]); > } > > -scott The assumption in vsi_reset_stats() is that the device is up and running normally, in which case all of the rings will be allocated, so we only need to check the first one. OTOH, in free_rx_resources can be called in case of an allocation failure, in which case you could conceivably have some rings allocated and some not. -Mitch -- 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 ea76134..5cdc67c 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_main.c +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c @@ -422,7 +422,7 @@ void i40e_vsi_reset_stats(struct i40e_vsi *vsi) memset(&vsi->net_stats_offsets, 0, sizeof(vsi->net_stats_offsets)); memset(&vsi->eth_stats, 0, sizeof(vsi->eth_stats)); memset(&vsi->eth_stats_offsets, 0, sizeof(vsi->eth_stats_offsets)); - if (vsi->rx_rings) + if (vsi->rx_rings && vsi->rx_rings[0]) { for (i = 0; i < vsi->num_queue_pairs; i++) { memset(&vsi->rx_rings[i]->stats, 0 , sizeof(vsi->rx_rings[i]->stats)); @@ -433,6 +433,7 @@ void i40e_vsi_reset_stats(struct i40e_vsi *vsi) memset(&vsi->tx_rings[i]->tx_stats, 0, sizeof(vsi->tx_rings[i]->tx_stats)); } + } vsi->stat_offsets_loaded = false; } @@ -2067,8 +2068,11 @@ static void i40e_vsi_free_tx_resources(struct i40e_vsi *vsi) { int i; + if (!vsi->tx_rings) + return; + for (i = 0; i < vsi->num_queue_pairs; i++) - if (vsi->tx_rings[i]->desc) + if (vsi->tx_rings[i] && vsi->tx_rings[i]->desc) i40e_free_tx_resources(vsi->tx_rings[i]); } @@ -2101,8 +2105,11 @@ static void i40e_vsi_free_rx_resources(struct i40e_vsi *vsi) { int i; + if (!vsi->rx_rings) + return; + for (i = 0; i < vsi->num_queue_pairs; i++) - if (vsi->rx_rings[i]->desc) + if (vsi->rx_rings[i] && vsi->rx_rings[i]->desc) i40e_free_rx_resources(vsi->rx_rings[i]); } @@ -5349,7 +5356,7 @@ static void i40e_vsi_clear_rings(struct i40e_vsi *vsi) { int i; - if (vsi->tx_rings[0]) { + if (vsi->tx_rings && vsi->tx_rings[0]) { for (i = 0; i < vsi->alloc_queue_pairs; i++) { kfree_rcu(vsi->tx_rings[i], rcu); vsi->tx_rings[i] = NULL; diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c index e81e5bf..bc72f4e 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c +++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c @@ -1715,6 +1715,7 @@ dma_error: static inline int __i40e_maybe_stop_tx(struct i40e_ring *tx_ring, int size) { netif_stop_subqueue(tx_ring->netdev, tx_ring->queue_index); + /* Memory barrier before checking head and tail */ smp_mb(); /* Check again in a case another CPU has just made room available. */