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@xxxxxxxxxxxxxxxx>
>>>
>> 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@xxxxxxxxxxxxxxxx>
>> 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@xxxxxxxxxxxxxxxx>
>> Signed-off-by: Doug Ledford <dledford@xxxxxxxxxx>
>>
>> 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);
>> }
>>
>
Attachment:
signature.asc
Description: OpenPGP digital signature
