Re: RFC: net: allow to propagate errors through ->ndo_hard_start_xmit()

[Patrick McHardy <kaber@trash.net>]

Jarek Poplawski wrote:
> On Tue, Nov 10, 2009 at 06:31:27PM +0100, Patrick McHardy wrote:
>>
>>>> - I'm not sure the error handling in dev_hard_start_xmit() for GSO
>>>>   skbs is optimal. When the driver returns an error, it is assumed
>>>>   the current segment has been freed. The patch then frees the
>>>>   entire GSO skb, including all remaining segments. Alternatively
>>>>   it could try to transmit the remaining segments later.
>>> Anyway, it seems this freeing should be described in the changelog,
>>> if not moved to a separate patch, since it fixes another problem,
>>> unless I forgot something.
>> What other problem are you refering to? I'm not aware of any
>> problems in the existing function.
> 
> This patch is about propagating errors, so it's not clear why there
> are some additional kfrees mixed with this. (But I see it's explained
> below.)

Well, to handle now propagated errors :) But sure, I'll fix up
the changelog when I return from dinner.

>>>>  	if (likely(!skb->next)) {
>>>>  		if (!list_empty(&ptype_all))
>>>> @@ -1804,6 +1804,8 @@ gso:
>>>>  		nskb->next = NULL;
>>>>  		rc = ops->ndo_start_xmit(nskb, dev);
>>>>  		if (unlikely(rc != NETDEV_TX_OK)) {
>>>> +			if (rc & ~NETDEV_TX_MASK)
>>>> +				goto out_kfree_gso_skb;
>>> If e.g. (rc == NETDEV_TX_OK | NET_XMIT_CN), why exactly is this freeing
>>> necessary now?
>>>
>>> Is e.g. (rc == NETDEV_TX_BUSY | NET_XMIT_CN) legal? If so, there would
>>> be use after kfree, I guess. Otherwise, it should be documented above
>>> (and maybe checked somewhere as well).
>> NET_XMIT_CN is a valid return value, yes. But its not freeing the
>> transmitted segment but the remaining ones. Its not strictly
>> necessary, but its the easiest way to treat all errors similar.
>> Otherwise you get complicated cases, f.i. when the driver returns
>> NET_XMIT_CN for the first segment and NETDEV_TX_OK for the
>> remaining ones.
> 
> It should be in the changelog and maybe a comment too. Even if it's
> right it's a change of functionality/behavior here.
> 
> I still don't know if/why (rc == NETDEV_TX_BUSY | NET_XMIT_CN) is
> OK. IMHO skb will be requeued after kfree here.

Ah I misread. NETDEV_TX_BUSY | NET_XMIT_CN is not valid. The
return value can be either a NETDEV_TX code, a NET_XMIT code
or an errno code. NETDEV_TX_OK, NET_XMIT_SUCCESS and no error
(errno) all have the value zero.

>>>>  			nskb->next = skb->next;
>>>>  			skb->next = nskb;
>>>>  			return rc;
>>>> @@ -1813,11 +1815,14 @@ gso:
>>>>  			return NETDEV_TX_BUSY;
>>>>  	} while (skb->next);
>>>>  
>>>> -	skb->destructor = DEV_GSO_CB(skb)->destructor;
>>>> +	rc = NETDEV_TX_OK;
>>> When is (rc != NETDEV_TX_OK) possible in this place?
>> Its gone in the current version.
> 
> Why don't you send the current version?

I did 2 hours ago :)