Signed-off-by: Wei Liu <wei.liu2@citrix.com> --- drivers/net/xen-netfront.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c index 7ffa43b..5527663 100644 --- a/drivers/net/xen-netfront.c +++ b/drivers/net/xen-netfront.c @@ -537,7 +537,6 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) struct netfront_info *np = netdev_priv(dev); struct netfront_stats *stats = this_cpu_ptr(np->stats); struct xen_netif_tx_request *tx; - struct xen_netif_extra_info *extra; char *data = skb->data; RING_IDX i; grant_ref_t ref; @@ -581,7 +580,6 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) tx->gref = np->grant_tx_ref[id] = ref; tx->offset = offset; tx->size = len; - extra = NULL; tx->flags = 0; if (skb->ip_summed == CHECKSUM_PARTIAL) @@ -597,10 +595,7 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) gso = (struct xen_netif_extra_info *) RING_GET_REQUEST(&np->tx, ++i); - if (extra) - extra->flags |= XEN_NETIF_EXTRA_FLAG_MORE; - else - tx->flags |= XEN_NETTXF_extra_info; + tx->flags |= XEN_NETTXF_extra_info; gso->u.gso.size = skb_shinfo(skb)->gso_size; gso->u.gso.type = XEN_NETIF_GSO_TYPE_TCPV4; @@ -609,7 +604,6 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) gso->type = XEN_NETIF_EXTRA_TYPE_GSO; gso->flags = 0; - extra = gso; } np->tx.req_prod_pvt = i + 1; -- 1.7.10.4
Ian Campbell
2013-Mar-18 11:42 UTC
Re: [PATCH 1/4] xen-netfront: remove unused variable `extra''
On Mon, 2013-03-18 at 10:35 +0000, Wei Liu wrote: I think a few more words are needed here since from the code you are removing it seems very much like gso is used for something. If you have a proof that the "extra = gso" case is never hit then please explain it. Perhaps a reference to the removal of the last user? Or maybe it is the case that it should be used and the bug is that it isn''t?> Signed-off-by: Wei Liu <wei.liu2@citrix.com> > --- > drivers/net/xen-netfront.c | 8 +------- > 1 file changed, 1 insertion(+), 7 deletions(-) > > diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c > index 7ffa43b..5527663 100644 > --- a/drivers/net/xen-netfront.c > +++ b/drivers/net/xen-netfront.c > @@ -537,7 +537,6 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) > struct netfront_info *np = netdev_priv(dev); > struct netfront_stats *stats = this_cpu_ptr(np->stats); > struct xen_netif_tx_request *tx; > - struct xen_netif_extra_info *extra; > char *data = skb->data; > RING_IDX i; > grant_ref_t ref; > @@ -581,7 +580,6 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) > tx->gref = np->grant_tx_ref[id] = ref; > tx->offset = offset; > tx->size = len; > - extra = NULL; > > tx->flags = 0; > if (skb->ip_summed == CHECKSUM_PARTIAL) > @@ -597,10 +595,7 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) > gso = (struct xen_netif_extra_info *) > RING_GET_REQUEST(&np->tx, ++i); > > - if (extra) > - extra->flags |= XEN_NETIF_EXTRA_FLAG_MORE; > - else > - tx->flags |= XEN_NETTXF_extra_info; > + tx->flags |= XEN_NETTXF_extra_info; > > gso->u.gso.size = skb_shinfo(skb)->gso_size; > gso->u.gso.type = XEN_NETIF_GSO_TYPE_TCPV4; > @@ -609,7 +604,6 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) > > gso->type = XEN_NETIF_EXTRA_TYPE_GSO; > gso->flags = 0; > - extra = gso; > } > > np->tx.req_prod_pvt = i + 1;
Wei Liu
2013-Mar-18 12:04 UTC
Re: [PATCH 1/4] xen-netfront: remove unused variable `extra''
On Mon, 2013-03-18 at 11:42 +0000, Ian Campbell wrote:> On Mon, 2013-03-18 at 10:35 +0000, Wei Liu wrote: > > I think a few more words are needed here since from the code you are > removing it seems very much like gso is used for something. If you have > a proof that the "extra = gso" case is never hit then please explain it. > Perhaps a reference to the removal of the last user? > > Or maybe it is the case that it should be used and the bug is that it > isn''t? >Looks like the latter one. ''extra'' field should be used to get hold of the last extra info in the ring. ;-) But, the only extra info in upstream kernel is XEN_NETIF_EXTRA_TYPE_GSO, so there''s really no other extra info in the ring at that point. Could it be possible that it is something from classic Xen kernel? Wei.> > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > > --- > > drivers/net/xen-netfront.c | 8 +------- > > 1 file changed, 1 insertion(+), 7 deletions(-) > > > > diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c > > index 7ffa43b..5527663 100644 > > --- a/drivers/net/xen-netfront.c > > +++ b/drivers/net/xen-netfront.c > > @@ -537,7 +537,6 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) > > struct netfront_info *np = netdev_priv(dev); > > struct netfront_stats *stats = this_cpu_ptr(np->stats); > > struct xen_netif_tx_request *tx; > > - struct xen_netif_extra_info *extra; > > char *data = skb->data; > > RING_IDX i; > > grant_ref_t ref; > > @@ -581,7 +580,6 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) > > tx->gref = np->grant_tx_ref[id] = ref; > > tx->offset = offset; > > tx->size = len; > > - extra = NULL; > > > > tx->flags = 0; > > if (skb->ip_summed == CHECKSUM_PARTIAL) > > @@ -597,10 +595,7 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) > > gso = (struct xen_netif_extra_info *) > > RING_GET_REQUEST(&np->tx, ++i); > > > > - if (extra) > > - extra->flags |= XEN_NETIF_EXTRA_FLAG_MORE; > > - else > > - tx->flags |= XEN_NETTXF_extra_info; > > + tx->flags |= XEN_NETTXF_extra_info; > > > > gso->u.gso.size = skb_shinfo(skb)->gso_size; > > gso->u.gso.type = XEN_NETIF_GSO_TYPE_TCPV4; > > @@ -609,7 +604,6 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) > > > > gso->type = XEN_NETIF_EXTRA_TYPE_GSO; > > gso->flags = 0; > > - extra = gso; > > } > > > > np->tx.req_prod_pvt = i + 1; > >
Ian Campbell
2013-Mar-18 12:14 UTC
Re: [PATCH 1/4] xen-netfront: remove unused variable `extra''
On Mon, 2013-03-18 at 12:04 +0000, Wei Liu wrote:> On Mon, 2013-03-18 at 11:42 +0000, Ian Campbell wrote: > > On Mon, 2013-03-18 at 10:35 +0000, Wei Liu wrote: > > > > I think a few more words are needed here since from the code you are > > removing it seems very much like gso is used for something. If you have > > a proof that the "extra = gso" case is never hit then please explain it. > > Perhaps a reference to the removal of the last user? > > > > Or maybe it is the case that it should be used and the bug is that it > > isn''t? > > > > Looks like the latter one. ''extra'' field should be used to get hold of > the last extra info in the ring. ;-) > > But, the only extra info in upstream kernel is XEN_NETIF_EXTRA_TYPE_GSO, > so there''s really no other extra info in the ring at that point. Could > it be possible that it is something from classic Xen kernel?The classic kernel netfront has exactly the same code it seems and netif_extra_type_gso is the only one I''ve ever heard of. Maybe this extra thing is just redundant unless/until a second extra comes along. Ian.
annie li
2013-Mar-19 02:39 UTC
Re: [PATCH 1/4] xen-netfront: remove unused variable `extra''
On 2013-3-18 20:14, Ian Campbell wrote:> On Mon, 2013-03-18 at 12:04 +0000, Wei Liu wrote: >> On Mon, 2013-03-18 at 11:42 +0000, Ian Campbell wrote: >>> On Mon, 2013-03-18 at 10:35 +0000, Wei Liu wrote: >>> >>> I think a few more words are needed here since from the code you are >>> removing it seems very much like gso is used for something. If you have >>> a proof that the "extra = gso" case is never hit then please explain it. >>> Perhaps a reference to the removal of the last user? >>> >>> Or maybe it is the case that it should be used and the bug is that it >>> isn''t? >>> >> Looks like the latter one. ''extra'' field should be used to get hold of >> the last extra info in the ring. ;-) >> >> But, the only extra info in upstream kernel is XEN_NETIF_EXTRA_TYPE_GSO, >> so there''s really no other extra info in the ring at that point. Could >> it be possible that it is something from classic Xen kernel? > The classic kernel netfront has exactly the same code it seems and > netif_extra_type_gso is the only one I''ve ever heard of. > > Maybe this extra thing is just redundant unless/until a second extra > comes along.In our windows pv driver, we do not process this for GSO in tx path either. Maybe we ignored processing for some special GSO? BTW, what is XEN_NETIF_EXTRA_FLAG_MORE actually for? Backend only processes it in xen_netback_tx_build_gops, but netfront xmit path does not really set this flag. I did process it in rx path of my windows pv driver(linux netfront did that too), but it seems unnecessary since netback does not set this flag at all. Thanks Annie
James Harper
2013-Mar-19 03:02 UTC
Re: [PATCH 1/4] xen-netfront: remove unused variable `extra''
> > On 2013-3-18 20:14, Ian Campbell wrote: > > On Mon, 2013-03-18 at 12:04 +0000, Wei Liu wrote: > >> On Mon, 2013-03-18 at 11:42 +0000, Ian Campbell wrote: > >>> On Mon, 2013-03-18 at 10:35 +0000, Wei Liu wrote: > >>> > >>> I think a few more words are needed here since from the code you are > >>> removing it seems very much like gso is used for something. If you have > >>> a proof that the "extra = gso" case is never hit then please explain it. > >>> Perhaps a reference to the removal of the last user? > >>> > >>> Or maybe it is the case that it should be used and the bug is that it > >>> isn''t? > >>> > >> Looks like the latter one. ''extra'' field should be used to get hold of > >> the last extra info in the ring. ;-) > >> > >> But, the only extra info in upstream kernel is > XEN_NETIF_EXTRA_TYPE_GSO, > >> so there''s really no other extra info in the ring at that point. Could > >> it be possible that it is something from classic Xen kernel? > > The classic kernel netfront has exactly the same code it seems and > > netif_extra_type_gso is the only one I''ve ever heard of. > > > > Maybe this extra thing is just redundant unless/until a second extra > > comes along. > > In our windows pv driver, we do not process this for GSO in tx path > either. Maybe we ignored processing for some special GSO? > > BTW, what is XEN_NETIF_EXTRA_FLAG_MORE actually for? Backend only > processes it in xen_netback_tx_build_gops, but netfront xmit path does > not really set this flag. I did process it in rx path of my windows pv > driver(linux netfront did that too), but it seems unnecessary since > netback does not set this flag at all. >This flag is set to say if there is another ''extra'' ring entry. From netif.h: /* * This is the ''wire'' format for packets: * Request 1: netif_tx_request -- NETTXF_* (any flags) * [Request 2: netif_tx_extra] (only if request 1 has NETTXF_extra_info) * [Request 3: netif_tx_extra] (only if request 2 has XEN_NETIF_EXTRA_MORE) * Request 4: netif_tx_request -- NETTXF_more_data * Request 5: netif_tx_request -- NETTXF_more_data * ... * Request N: netif_tx_request -- 0 */ I think the only extra type is GSO so you''ll probably never see it, but that''s what it''s for. James
Paul Durrant
2013-Mar-19 09:28 UTC
Re: [PATCH 1/4] xen-netfront: remove unused variable `extra''
> -----Original Message----- > From: xen-devel-bounces@lists.xen.org [mailto:xen-devel- > bounces@lists.xen.org] On Behalf Of annie li > Sent: 19 March 2013 02:39 > To: Ian Campbell > Cc: netdev@vger.kernel.org; konrad.wilk@oracle.com; Wei Liu; xen- > devel@lists.xen.org > Subject: Re: [Xen-devel] [PATCH 1/4] xen-netfront: remove unused variable > `extra'' > > > On 2013-3-18 20:14, Ian Campbell wrote: > > On Mon, 2013-03-18 at 12:04 +0000, Wei Liu wrote: > >> On Mon, 2013-03-18 at 11:42 +0000, Ian Campbell wrote: > >>> On Mon, 2013-03-18 at 10:35 +0000, Wei Liu wrote: > >>> > >>> I think a few more words are needed here since from the code you are > >>> removing it seems very much like gso is used for something. If you have > >>> a proof that the "extra = gso" case is never hit then please explain it. > >>> Perhaps a reference to the removal of the last user? > >>> > >>> Or maybe it is the case that it should be used and the bug is that it > >>> isn''t? > >>> > >> Looks like the latter one. ''extra'' field should be used to get hold of > >> the last extra info in the ring. ;-) > >> > >> But, the only extra info in upstream kernel is > XEN_NETIF_EXTRA_TYPE_GSO, > >> so there''s really no other extra info in the ring at that point. Could > >> it be possible that it is something from classic Xen kernel? > > The classic kernel netfront has exactly the same code it seems and > > netif_extra_type_gso is the only one I''ve ever heard of. > > > > Maybe this extra thing is just redundant unless/until a second extra > > comes along. > > In our windows pv driver, we do not process this for GSO in tx path > either. Maybe we ignored processing for some special GSO? > > BTW, what is XEN_NETIF_EXTRA_FLAG_MORE actually for? Backend only > processes it in xen_netback_tx_build_gops, but netfront xmit path does > not really set this flag. I did process it in rx path of my windows pv > driver(linux netfront did that too), but it seems unnecessary since > netback does not set this flag at all. >The flag is there to denote the existence of an ''extra'' segment in the packet. The ''extra'' segment goes after the 1st segment and specifies metadata such as the GSO type (TCPv4 is the only one at the moment but we''ll need TCPv6 very shortly) and the MSS. Extra segments are certainly not redundant; the Citrix Windows PV drivers send TSOs using them and handle LRO using them too. Paul
annie li
2013-Mar-19 09:53 UTC
Re: [PATCH 1/4] xen-netfront: remove unused variable `extra''
On 2013-3-19 17:28, Paul Durrant wrote:>> -----Original Message----- >> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel- >> bounces@lists.xen.org] On Behalf Of annie li >> Sent: 19 March 2013 02:39 >> To: Ian Campbell >> Cc: netdev@vger.kernel.org; konrad.wilk@oracle.com; Wei Liu; xen- >> devel@lists.xen.org >> Subject: Re: [Xen-devel] [PATCH 1/4] xen-netfront: remove unused variable >> `extra'' >> >> >> On 2013-3-18 20:14, Ian Campbell wrote: >>> On Mon, 2013-03-18 at 12:04 +0000, Wei Liu wrote: >>>> On Mon, 2013-03-18 at 11:42 +0000, Ian Campbell wrote: >>>>> On Mon, 2013-03-18 at 10:35 +0000, Wei Liu wrote: >>>>> >>>>> I think a few more words are needed here since from the code you are >>>>> removing it seems very much like gso is used for something. If you have >>>>> a proof that the "extra = gso" case is never hit then please explain it. >>>>> Perhaps a reference to the removal of the last user? >>>>> >>>>> Or maybe it is the case that it should be used and the bug is that it >>>>> isn''t? >>>>> >>>> Looks like the latter one. ''extra'' field should be used to get hold of >>>> the last extra info in the ring. ;-) >>>> >>>> But, the only extra info in upstream kernel is >> XEN_NETIF_EXTRA_TYPE_GSO, >>>> so there''s really no other extra info in the ring at that point. Could >>>> it be possible that it is something from classic Xen kernel? >>> The classic kernel netfront has exactly the same code it seems and >>> netif_extra_type_gso is the only one I''ve ever heard of. >>> >>> Maybe this extra thing is just redundant unless/until a second extra >>> comes along. >> In our windows pv driver, we do not process this for GSO in tx path >> either. Maybe we ignored processing for some special GSO? >> >> BTW, what is XEN_NETIF_EXTRA_FLAG_MORE actually for? Backend only >> processes it in xen_netback_tx_build_gops, but netfront xmit path does >> not really set this flag. I did process it in rx path of my windows pv >> driver(linux netfront did that too), but it seems unnecessary since >> netback does not set this flag at all. >> > The flag is there to denote the existence of an ''extra'' segment in the packet. The ''extra'' segment goes after the 1st segment and specifies metadata such as the GSO type (TCPv4 is the only one at the moment but we''ll need TCPv6 very shortly) and the MSS.For TCPv4 GSO, it seems one extra info request(NETTXF_extra_info) is enough in my winpv driver, and I did not process the XEN_NETIF_EXTRA_MORE. Do you create two extra info requests for bothTCPv4 and TCPv6 GSO like following? * [Request 2: netif_tx_extra] (only if request 1 has NETTXF_extra_info) * [Request 3: netif_tx_extra] (only if request 2 has XEN_NETIF_EXTRA_MORE)> Extra segments are certainly not redundant; the Citrix Windows PV drivers send TSOs using them and handle LRO using them too.About the LRO, upstream netback does not create any response with XEN_NETIF_EXTRA_MORE, so I assume your dom0 did such process? Thanks Annie
Paul Durrant
2013-Mar-19 10:03 UTC
Re: [PATCH 1/4] xen-netfront: remove unused variable `extra''
> -----Original Message-----[snip]> For TCPv4 GSO, it seems one extra info request(NETTXF_extra_info) is > enough in my winpv driver, and I did not process the > XEN_NETIF_EXTRA_MORE. > Do you create two extra info requests for bothTCPv4 and TCPv6 GSO like > following? > > * [Request 2: netif_tx_extra] (only if request 1 has NETTXF_extra_info) > * [Request 3: netif_tx_extra] (only if request 2 has > XEN_NETIF_EXTRA_MORE) >No, I just use one. I don''t use XEN_NETIF_EXTRA_FLAG_MORE. I thought you were questioning the existence of extra segments rather than this flag. I guess I got the wrong end of the stick. I''ve not seen anything use XEN_NETIF_EXTRA_FLAG_MORE, but that''s not to say nothing will use it in the future. Clearly something is needed to indicate subsequent extra segments should they ever be needed. Paul> > > Extra segments are certainly not redundant; the Citrix Windows PV drivers > send TSOs using them and handle LRO using them too. > > About the LRO, upstream netback does not create any response with > XEN_NETIF_EXTRA_MORE, so I assume your dom0 did such process? > > Thanks > Annie
Wei Liu
2013-Mar-19 15:26 UTC
Re: [PATCH 1/4] xen-netfront: remove unused variable `extra''
On Tue, 2013-03-19 at 09:28 +0000, Paul Durrant wrote:> > -----Original Message----- > > From: xen-devel-bounces@lists.xen.org [mailto:xen-devel- > > bounces@lists.xen.org] On Behalf Of annie li > > Sent: 19 March 2013 02:39 > > To: Ian Campbell > > Cc: netdev@vger.kernel.org; konrad.wilk@oracle.com; Wei Liu; xen- > > devel@lists.xen.org > > Subject: Re: [Xen-devel] [PATCH 1/4] xen-netfront: remove unused variable > > `extra'' > > > > > > On 2013-3-18 20:14, Ian Campbell wrote: > > > On Mon, 2013-03-18 at 12:04 +0000, Wei Liu wrote: > > >> On Mon, 2013-03-18 at 11:42 +0000, Ian Campbell wrote: > > >>> On Mon, 2013-03-18 at 10:35 +0000, Wei Liu wrote: > > >>> > > >>> I think a few more words are needed here since from the code you are > > >>> removing it seems very much like gso is used for something. If you have > > >>> a proof that the "extra = gso" case is never hit then please explain it. > > >>> Perhaps a reference to the removal of the last user? > > >>> > > >>> Or maybe it is the case that it should be used and the bug is that it > > >>> isn''t? > > >>> > > >> Looks like the latter one. ''extra'' field should be used to get hold of > > >> the last extra info in the ring. ;-) > > >> > > >> But, the only extra info in upstream kernel is > > XEN_NETIF_EXTRA_TYPE_GSO, > > >> so there''s really no other extra info in the ring at that point. Could > > >> it be possible that it is something from classic Xen kernel? > > > The classic kernel netfront has exactly the same code it seems and > > > netif_extra_type_gso is the only one I''ve ever heard of. > > > > > > Maybe this extra thing is just redundant unless/until a second extra > > > comes along. > > > > In our windows pv driver, we do not process this for GSO in tx path > > either. Maybe we ignored processing for some special GSO? > > > > BTW, what is XEN_NETIF_EXTRA_FLAG_MORE actually for? Backend only > > processes it in xen_netback_tx_build_gops, but netfront xmit path does > > not really set this flag. I did process it in rx path of my windows pv > > driver(linux netfront did that too), but it seems unnecessary since > > netback does not set this flag at all. > > > > The flag is there to denote the existence of an ''extra'' segment in the packet. The ''extra'' segment goes after the 1st segment and specifies metadata such as the GSO type (TCPv4 is the only one at the moment but we''ll need TCPv6 very shortly) and the MSS. > Extra segments are certainly not redundant; the Citrix Windows PV drivers send TSOs using them and handle LRO using them too. >I think Ian''s (and my) idea of redundant is that this ''extra'' variable is never used in the code now and causes confusion. It can be removed now and add back in the future if necessary. Wei.> Paul
Ian Campbell
2013-Apr-09 14:28 UTC
Re: [PATCH 1/4] xen-netfront: remove unused variable `extra''
(apologies for the late reply, I''ve been away) On Tue, 2013-03-19 at 15:26 +0000, Wei Liu wrote:> I think Ian''s (and my) idea of redundant is that this ''extra'' variable > is never used in the code now and causes confusion. It can be removed > now and add back in the future if necessary.Right, the "extra" I was questioning at the top was a local variable in the Linux code not the XEN_NETIF_EXTRA_FLAG_MORE thing. Although the variable was related to the handling of that flag it currently was written and then never read... Ian.