| Message ID | 1371653272-11703-3-git-send-email-vyasevic@redhat.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, Jun 19, 2013 at 10:47:52AM -0400, Vlad Yasevich wrote: > When macvtap forwards skb to its tap, it needs to check > if GSO needs to be performed. This is necessary > when the HW device performed GRO, but the guest reading > from the tap does not support it (ex: Windows 7). > > Signed-off-by: Vlad Yasevich <vyasevic@redhat.com> > --- > drivers/net/macvtap.c | 26 +++++++++++++++++++++++++- > 1 file changed, 25 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c > index 09f0b1f..698f613 100644 > --- a/drivers/net/macvtap.c > +++ b/drivers/net/macvtap.c > @@ -291,13 +291,37 @@ static void macvtap_del_queues(struct net_device *dev) > static int macvtap_forward(struct net_device *dev, struct sk_buff *skb) > { > struct macvtap_queue *q = macvtap_get_queue(dev, skb); > + netdev_features_t features; > if (!q) > goto drop; > > if (skb_queue_len(&q->sk.sk_receive_queue) >= dev->tx_queue_len) > goto drop; > > - skb_queue_tail(&q->sk.sk_receive_queue, skb); > + features = netif_skb_features(skb); Confused. skb->dev here points to the source macvlan so features are wrong - we need destination features, no? > + if (netif_needs_gso(skb, features)) { > + struct sk_buff *segs = skb_gso_segment(skb, features); I'd prefer a different name for this variable. skb_seg? > + > + if (IS_ERR(segs)) > + goto drop; > + > + if (!segs) { > + skb_queue_tail(&q->sk.sk_receive_queue, skb); > + goto wake_up; > + } > + > + kfree_skb(skb); > + while (segs) { > + struct sk_buff *nskb = segs->next; > + > + segs->next = NULL; > + skb_queue_tail(&q->sk.sk_receive_queue, segs); > + segs = nskb; > + } > + } else > + skb_queue_tail(&q->sk.sk_receive_queue, skb); > + > +wake_up: > wake_up_interruptible_poll(sk_sleep(&q->sk), POLLIN | POLLRDNORM | POLLRDBAND); > return NET_RX_SUCCESS; > > -- > 1.8.1.4 -- 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 06/19/2013 10:47 AM, Vlad Yasevich wrote: > When macvtap forwards skb to its tap, it needs to check > if GSO needs to be performed. This is necessary > when the HW device performed GRO, but the guest reading > from the tap does not support it (ex: Windows 7). > > Signed-off-by: Vlad Yasevich <vyasevic@redhat.com> > --- > drivers/net/macvtap.c | 26 +++++++++++++++++++++++++- > 1 file changed, 25 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c > index 09f0b1f..698f613 100644 > --- a/drivers/net/macvtap.c > +++ b/drivers/net/macvtap.c > @@ -291,13 +291,37 @@ static void macvtap_del_queues(struct net_device *dev) > static int macvtap_forward(struct net_device *dev, struct sk_buff *skb) > { > struct macvtap_queue *q = macvtap_get_queue(dev, skb); > + netdev_features_t features; > if (!q) > goto drop; > > if (skb_queue_len(&q->sk.sk_receive_queue) >= dev->tx_queue_len) > goto drop; > > - skb_queue_tail(&q->sk.sk_receive_queue, skb); > + features = netif_skb_features(skb); Ooops.. This is the wrong patch... -vlad > + if (netif_needs_gso(skb, features)) { > + struct sk_buff *segs = skb_gso_segment(skb, features); > + > + if (IS_ERR(segs)) > + goto drop; > + > + if (!segs) { > + skb_queue_tail(&q->sk.sk_receive_queue, skb); > + goto wake_up; > + } > + > + kfree_skb(skb); > + while (segs) { > + struct sk_buff *nskb = segs->next; > + > + segs->next = NULL; > + skb_queue_tail(&q->sk.sk_receive_queue, segs); > + segs = nskb; > + } > + } else > + skb_queue_tail(&q->sk.sk_receive_queue, skb); > + > +wake_up: > wake_up_interruptible_poll(sk_sleep(&q->sk), POLLIN | POLLRDNORM | POLLRDBAND); > return NET_RX_SUCCESS; > > -- 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 06/19/2013 11:30 AM, Michael S. Tsirkin wrote: > On Wed, Jun 19, 2013 at 10:47:52AM -0400, Vlad Yasevich wrote: >> When macvtap forwards skb to its tap, it needs to check >> if GSO needs to be performed. This is necessary >> when the HW device performed GRO, but the guest reading >> from the tap does not support it (ex: Windows 7). >> >> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com> >> --- >> drivers/net/macvtap.c | 26 +++++++++++++++++++++++++- >> 1 file changed, 25 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c >> index 09f0b1f..698f613 100644 >> --- a/drivers/net/macvtap.c >> +++ b/drivers/net/macvtap.c >> @@ -291,13 +291,37 @@ static void macvtap_del_queues(struct net_device *dev) >> static int macvtap_forward(struct net_device *dev, struct sk_buff *skb) >> { >> struct macvtap_queue *q = macvtap_get_queue(dev, skb); >> + netdev_features_t features; >> if (!q) >> goto drop; >> >> if (skb_queue_len(&q->sk.sk_receive_queue) >= dev->tx_queue_len) >> goto drop; >> >> - skb_queue_tail(&q->sk.sk_receive_queue, skb); >> + features = netif_skb_features(skb); > > Confused. skb->dev here points to the source macvlan > so features are wrong - we need destination features, no? yes and no.... Thanks for pointing this out as this is actually the wrong patch. So, to answer your question, in the case of receive, the device is already the destination device. In the case of broadcast forward, the skb->dev is actually null and the 'correct' patch does: skb->dev = dev; > >> + if (netif_needs_gso(skb, features)) { >> + struct sk_buff *segs = skb_gso_segment(skb, features); > > I'd prefer a different name for this variable. > skb_seg? > OK. -vlad >> + >> + if (IS_ERR(segs)) >> + goto drop; >> + >> + if (!segs) { >> + skb_queue_tail(&q->sk.sk_receive_queue, skb); >> + goto wake_up; >> + } >> + >> + kfree_skb(skb); >> + while (segs) { >> + struct sk_buff *nskb = segs->next; >> + >> + segs->next = NULL; >> + skb_queue_tail(&q->sk.sk_receive_queue, segs); >> + segs = nskb; >> + } > > >> + } else >> + skb_queue_tail(&q->sk.sk_receive_queue, skb); >> + >> +wake_up: >> wake_up_interruptible_poll(sk_sleep(&q->sk), POLLIN | POLLRDNORM | POLLRDBAND); >> return NET_RX_SUCCESS; >> >> -- >> 1.8.1.4 -- 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
Hello. On 06/19/2013 06:47 PM, Vlad Yasevich wrote: > When macvtap forwards skb to its tap, it needs to check > if GSO needs to be performed. This is necessary > when the HW device performed GRO, but the guest reading > from the tap does not support it (ex: Windows 7). > Signed-off-by: Vlad Yasevich <vyasevic@redhat.com> > --- > drivers/net/macvtap.c | 26 +++++++++++++++++++++++++- > 1 file changed, 25 insertions(+), 1 deletion(-) > diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c > index 09f0b1f..698f613 100644 > --- a/drivers/net/macvtap.c > +++ b/drivers/net/macvtap.c > @@ -291,13 +291,37 @@ static void macvtap_del_queues(struct net_device *dev) > static int macvtap_forward(struct net_device *dev, struct sk_buff *skb) > { > struct macvtap_queue *q = macvtap_get_queue(dev, skb); > + netdev_features_t features; > if (!q) > goto drop; > > if (skb_queue_len(&q->sk.sk_receive_queue) >= dev->tx_queue_len) > goto drop; > > - skb_queue_tail(&q->sk.sk_receive_queue, skb); > + features = netif_skb_features(skb); > + if (netif_needs_gso(skb, features)) { > + struct sk_buff *segs = skb_gso_segment(skb, features); > + > + if (IS_ERR(segs)) > + goto drop; > + > + if (!segs) { > + skb_queue_tail(&q->sk.sk_receive_queue, skb); > + goto wake_up; > + } > + > + kfree_skb(skb); > + while (segs) { > + struct sk_buff *nskb = segs->next; > + > + segs->next = NULL; > + skb_queue_tail(&q->sk.sk_receive_queue, segs); > + segs = nskb; > + } > + } else > + skb_queue_tail(&q->sk.sk_receive_queue, skb); According to Documentation/CodingStyle, *else* branch should have {} if the other branch has it (and vice versa). 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
diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c index 09f0b1f..698f613 100644 --- a/drivers/net/macvtap.c +++ b/drivers/net/macvtap.c @@ -291,13 +291,37 @@ static void macvtap_del_queues(struct net_device *dev) static int macvtap_forward(struct net_device *dev, struct sk_buff *skb) { struct macvtap_queue *q = macvtap_get_queue(dev, skb); + netdev_features_t features; if (!q) goto drop; if (skb_queue_len(&q->sk.sk_receive_queue) >= dev->tx_queue_len) goto drop; - skb_queue_tail(&q->sk.sk_receive_queue, skb); + features = netif_skb_features(skb); + if (netif_needs_gso(skb, features)) { + struct sk_buff *segs = skb_gso_segment(skb, features); + + if (IS_ERR(segs)) + goto drop; + + if (!segs) { + skb_queue_tail(&q->sk.sk_receive_queue, skb); + goto wake_up; + } + + kfree_skb(skb); + while (segs) { + struct sk_buff *nskb = segs->next; + + segs->next = NULL; + skb_queue_tail(&q->sk.sk_receive_queue, segs); + segs = nskb; + } + } else + skb_queue_tail(&q->sk.sk_receive_queue, skb); + +wake_up: wake_up_interruptible_poll(sk_sleep(&q->sk), POLLIN | POLLRDNORM | POLLRDBAND); return NET_RX_SUCCESS;
When macvtap forwards skb to its tap, it needs to check if GSO needs to be performed. This is necessary when the HW device performed GRO, but the guest reading from the tap does not support it (ex: Windows 7). Signed-off-by: Vlad Yasevich <vyasevic@redhat.com> --- drivers/net/macvtap.c | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-)