| Message ID | be44b670331802b56498b008af89437b568bbd93.camel@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Doug Ledford |
| Headers | show |
Yes, sorry for that, I should have based the patch on wip/dl-for-next. It looks much better without the new_path variable. But why the else branch with init_path_rec for an existing path is also gone? On 22/05/18 19:51, Doug Ledford wrote: > On Tue, 2018-05-22 at 16:00 +0200, Evgenii Smirnov wrote: >> In unicast_arp_send function there is an inconsistency in error handling >> of path_rec_start call. If path_rec_start is called because of an absent >> ah field, skb will be dropped. But if it is called on a creation of a new path, >> or if the path is invalid, skb will be added to the tail of path queue. >> In case of a new path it will be dropped on path_free, but in case >> of invalid path it can stay in the queue forever. >> >> This patch unifies the behavior, dropping skb in all cases >> of path_rec_start failure. >> >> Signed-off-by: Evgenii Smirnov <evgenii.smirnov@profitbricks.com> >> > Your patch wouldn't apply (would have needed to be rebased on top of my > final patch). So, I hand applied your patch, and simplified things even > a little further in the process (if we allocate a new path, just add it > right away and greatly simplify the code path, allowing us to entirely > eliminate the new_path variable and unwind sequences). Here it is for > review: > > commit 5755dd0e647eecaf3c32af858e9ef69db199b0a7 (HEAD -> k.o/wip/dl-for-next) > Author: Evgenii Smirnov <evgenii.smirnov@profitbricks.com> > Date: Tue May 22 16:00:47 2018 +0200 > > RDMA/ipoib: drop skb on path record lookup failure > > In unicast_arp_send function there is an inconsistency in error handling > of path_rec_start call. If path_rec_start is called because of an absent > ah field, skb will be dropped. But if it is called on a creation of a > new path, or if the path is invalid, skb will be added to the tail of > path queue. In case of a new path it will be dropped on path_free, but > in case of invalid path it can stay in the queue forever. > > This patch unifies the behavior, dropping skb in all cases > of path_rec_start failure. > > Signed-off-by: Evgenii Smirnov <evgenii.smirnov@profitbricks.com> > Signed-off-by: Doug Ledford <dledford@redhat.com> > > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c > index 788bb9573f1f..b84f9e8f881d 100644 > --- a/drivers/infiniband/ulp/ipoib/ipoib_main.c > +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c > @@ -1054,62 +1054,36 @@ static void unicast_arp_send(struct sk_buff *skb, struct net_device *dev, > > path = __path_find(dev, phdr->hwaddr + 4); > if (!path || !path->ah || !path->ah->valid) { > - int new_path = 0; > - > if (!path) { > path = path_rec_create(dev, phdr->hwaddr + 4); > - new_path = 1; > + if (!path) > + goto drop_and_unlock; > + __path_add(dev, path); > + } > + if (!path->query && path_rec_start(dev, path)) { > + goto drop_and_unlock; > } > - if (path) { > - if (!new_path) > - /* make sure there is no changes in the existing path record */ > - init_path_rec(priv, path, phdr->hwaddr + 4); > - > - if (skb_queue_len(&path->queue) < IPOIB_MAX_PATH_REC_QUEUE) { > - push_pseudo_header(skb, phdr->hwaddr); > - __skb_queue_tail(&path->queue, skb); > - } else { > - ++dev->stats.tx_dropped; > - dev_kfree_skb_any(skb); > - } > > - if (!path->query && path_rec_start(dev, path)) { > - spin_unlock_irqrestore(&priv->lock, flags); > - if (new_path) > - path_free(dev, path); > - return; > - } else > - __path_add(dev, path); > + if (skb_queue_len(&path->queue) < IPOIB_MAX_PATH_REC_QUEUE) { > + push_pseudo_header(skb, phdr->hwaddr); > + __skb_queue_tail(&path->queue, skb); > + goto unlock; > } else { > goto drop_and_unlock; > } > - > - spin_unlock_irqrestore(&priv->lock, flags); > - return; > - } > - > - if (path->ah && path->ah->valid) { > - ipoib_dbg(priv, "Send unicast ARP to %08x\n", > - be32_to_cpu(sa_path_get_dlid(&path->pathrec))); > - > - spin_unlock_irqrestore(&priv->lock, flags); > - path->ah->last_send = rn->send(dev, skb, path->ah->ah, > - IPOIB_QPN(phdr->hwaddr)); > - return; > - } else if ((path->query || !path_rec_start(dev, path)) && > - skb_queue_len(&path->queue) < IPOIB_MAX_PATH_REC_QUEUE) { > - push_pseudo_header(skb, phdr->hwaddr); > - __skb_queue_tail(&path->queue, skb); > - } else { > - goto drop_and_unlock; > } > > spin_unlock_irqrestore(&priv->lock, flags); > + ipoib_dbg(priv, "Send unicast ARP to %08x\n", > + be32_to_cpu(sa_path_get_dlid(&path->pathrec))); > + path->ah->last_send = rn->send(dev, skb, path->ah->ah, > + IPOIB_QPN(phdr->hwaddr)); > return; > > drop_and_unlock: > ++dev->stats.tx_dropped; > dev_kfree_skb_any(skb); > +unlock: > spin_unlock_irqrestore(&priv->lock, flags); > } > > -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 5/23/2018 7:57 AM, Evgenii Smirnov wrote: > Yes, sorry for that, I should have based the patch on wip/dl-for-next. > > It looks much better without the new_path variable. But why the else > branch with init_path_rec for an existing path is also gone? That was an oversight on my part. I fixed it before I pushed out my wip/dl-ipoib branch, so the fixed version of the patch is there. > > On 22/05/18 19:51, Doug Ledford wrote: >> On Tue, 2018-05-22 at 16:00 +0200, Evgenii Smirnov wrote: >>> In unicast_arp_send function there is an inconsistency in error handling >>> of path_rec_start call. If path_rec_start is called because of an absent >>> ah field, skb will be dropped. But if it is called on a creation of a >>> new path, >>> or if the path is invalid, skb will be added to the tail of path queue. >>> In case of a new path it will be dropped on path_free, but in case >>> of invalid path it can stay in the queue forever. >>> >>> This patch unifies the behavior, dropping skb in all cases >>> of path_rec_start failure. >>> >>> Signed-off-by: Evgenii Smirnov <evgenii.smirnov@profitbricks.com> >>> >> Your patch wouldn't apply (would have needed to be rebased on top of my >> final patch). So, I hand applied your patch, and simplified things even >> a little further in the process (if we allocate a new path, just add it >> right away and greatly simplify the code path, allowing us to entirely >> eliminate the new_path variable and unwind sequences). Here it is for >> review: >> >> commit 5755dd0e647eecaf3c32af858e9ef69db199b0a7 (HEAD -> >> k.o/wip/dl-for-next) >> Author: Evgenii Smirnov <evgenii.smirnov@profitbricks.com> >> Date: Tue May 22 16:00:47 2018 +0200 >> >> RDMA/ipoib: drop skb on path record lookup failure >> In unicast_arp_send function there is an inconsistency in >> error handling >> of path_rec_start call. If path_rec_start is called because of an >> absent >> ah field, skb will be dropped. But if it is called on a creation >> of a >> new path, or if the path is invalid, skb will be added to the >> tail of >> path queue. In case of a new path it will be dropped on >> path_free, but >> in case of invalid path it can stay in the queue forever. >> This patch unifies the behavior, dropping skb in all cases >> of path_rec_start failure. >> Signed-off-by: Evgenii Smirnov >> <evgenii.smirnov@profitbricks.com> >> Signed-off-by: Doug Ledford <dledford@redhat.com> >> >> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c >> b/drivers/infiniband/ulp/ipoib/ipoib_main.c >> index 788bb9573f1f..b84f9e8f881d 100644 >> --- a/drivers/infiniband/ulp/ipoib/ipoib_main.c >> +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c >> @@ -1054,62 +1054,36 @@ static void unicast_arp_send(struct sk_buff >> *skb, struct net_device *dev, >> path = __path_find(dev, phdr->hwaddr + 4); >> if (!path || !path->ah || !path->ah->valid) { >> - int new_path = 0; >> - >> if (!path) { >> path = path_rec_create(dev, phdr->hwaddr + 4); >> - new_path = 1; >> + if (!path) >> + goto drop_and_unlock; >> + __path_add(dev, path); >> + } >> + if (!path->query && path_rec_start(dev, path)) { >> + goto drop_and_unlock; >> } >> - if (path) { >> - if (!new_path) >> - /* make sure there is no changes in >> the existing path record */ >> - init_path_rec(priv, path, phdr->hwaddr >> + 4); >> - >> - if (skb_queue_len(&path->queue) < >> IPOIB_MAX_PATH_REC_QUEUE) { >> - push_pseudo_header(skb, phdr->hwaddr); >> - __skb_queue_tail(&path->queue, skb); >> - } else { >> - ++dev->stats.tx_dropped; >> - dev_kfree_skb_any(skb); >> - } >> - if (!path->query && path_rec_start(dev, >> path)) { >> - spin_unlock_irqrestore(&priv->lock, >> flags); >> - if (new_path) >> - path_free(dev, path); >> - return; >> - } else >> - __path_add(dev, path); >> + if (skb_queue_len(&path->queue) < >> IPOIB_MAX_PATH_REC_QUEUE) { >> + push_pseudo_header(skb, phdr->hwaddr); >> + __skb_queue_tail(&path->queue, skb); >> + goto unlock; >> } else { >> goto drop_and_unlock; >> } >> - >> - spin_unlock_irqrestore(&priv->lock, flags); >> - return; >> - } >> - >> - if (path->ah && path->ah->valid) { >> - ipoib_dbg(priv, "Send unicast ARP to %08x\n", >> - be32_to_cpu(sa_path_get_dlid(&path->pathrec))); >> - >> - spin_unlock_irqrestore(&priv->lock, flags); >> - path->ah->last_send = rn->send(dev, skb, path->ah->ah, >> - IPOIB_QPN(phdr->hwaddr)); >> - return; >> - } else if ((path->query || !path_rec_start(dev, path)) && >> - skb_queue_len(&path->queue) < >> IPOIB_MAX_PATH_REC_QUEUE) { >> - push_pseudo_header(skb, phdr->hwaddr); >> - __skb_queue_tail(&path->queue, skb); >> - } else { >> - goto drop_and_unlock; >> } >> spin_unlock_irqrestore(&priv->lock, flags); >> + ipoib_dbg(priv, "Send unicast ARP to %08x\n", >> + be32_to_cpu(sa_path_get_dlid(&path->pathrec))); >> + path->ah->last_send = rn->send(dev, skb, path->ah->ah, >> + IPOIB_QPN(phdr->hwaddr)); >> return; >> drop_and_unlock: >> ++dev->stats.tx_dropped; >> dev_kfree_skb_any(skb); >> +unlock: >> spin_unlock_irqrestore(&priv->lock, flags); >> } >> >
On Wed, 2018-05-23 at 10:45 -0400, Doug Ledford wrote: > On 5/23/2018 7:57 AM, Evgenii Smirnov wrote: > > Yes, sorry for that, I should have based the patch on wip/dl-for-next. > > > > It looks much better without the new_path variable. But why the else > > branch with init_path_rec for an existing path is also gone? > > That was an oversight on my part. I fixed it before I pushed out my > wip/dl-ipoib branch, so the fixed version of the patch is there. The final version of this has been merged into my wip/dl-for-next.
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c index 788bb9573f1f..b84f9e8f881d 100644 --- a/drivers/infiniband/ulp/ipoib/ipoib_main.c +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c @@ -1054,62 +1054,36 @@ static void unicast_arp_send(struct sk_buff *skb, struct net_device *dev, path = __path_find(dev, phdr->hwaddr + 4); if (!path || !path->ah || !path->ah->valid) { - int new_path = 0; - if (!path) { path = path_rec_create(dev, phdr->hwaddr + 4); - new_path = 1; + if (!path) + goto drop_and_unlock; + __path_add(dev, path); + } + if (!path->query && path_rec_start(dev, path)) { + goto drop_and_unlock; } - if (path) { - if (!new_path) - /* make sure there is no changes in the existing path record */ - init_path_rec(priv, path, phdr->hwaddr + 4); - - if (skb_queue_len(&path->queue) < IPOIB_MAX_PATH_REC_QUEUE) { - push_pseudo_header(skb, phdr->hwaddr); - __skb_queue_tail(&path->queue, skb); - } else { - ++dev->stats.tx_dropped; - dev_kfree_skb_any(skb); - } - if (!path->query && path_rec_start(dev, path)) { - spin_unlock_irqrestore(&priv->lock, flags); - if (new_path) - path_free(dev, path); - return; - } else - __path_add(dev, path); + if (skb_queue_len(&path->queue) < IPOIB_MAX_PATH_REC_QUEUE) { + push_pseudo_header(skb, phdr->hwaddr); + __skb_queue_tail(&path->queue, skb); + goto unlock; } else { goto drop_and_unlock; } - - spin_unlock_irqrestore(&priv->lock, flags); - return; - } - - if (path->ah && path->ah->valid) { - ipoib_dbg(priv, "Send unicast ARP to %08x\n", - be32_to_cpu(sa_path_get_dlid(&path->pathrec))); - - spin_unlock_irqrestore(&priv->lock, flags); - path->ah->last_send = rn->send(dev, skb, path->ah->ah, - IPOIB_QPN(phdr->hwaddr)); - return; - } else if ((path->query || !path_rec_start(dev, path)) && - skb_queue_len(&path->queue) < IPOIB_MAX_PATH_REC_QUEUE) { - push_pseudo_header(skb, phdr->hwaddr); - __skb_queue_tail(&path->queue, skb); - } else { - goto drop_and_unlock; } spin_unlock_irqrestore(&priv->lock, flags); + ipoib_dbg(priv, "Send unicast ARP to %08x\n", + be32_to_cpu(sa_path_get_dlid(&path->pathrec))); + path->ah->last_send = rn->send(dev, skb, path->ah->ah, + IPOIB_QPN(phdr->hwaddr)); return; drop_and_unlock: ++dev->stats.tx_dropped; dev_kfree_skb_any(skb); +unlock: spin_unlock_irqrestore(&priv->lock, flags); }