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

[Jarek Poplawski <jarkao2@gmail.com>]

On Tue, Nov 10, 2009 at 06:31:27PM +0100, Patrick McHardy wrote:
> Jarek Poplawski wrote:
> > On Mon, Nov 09, 2009 at 08:41:36PM +0100, Patrick McHardy wrote:
> >> I've updated my patch to propagate error values (errno and NET_XMIT
> >> codes) through ndo_hard_start_xmit() and incorporated the suggestions
> >> made last time, namely:
> >>
> >> - move slightly complicated return value check to inline function and
> >>   add a few comments
> >>
> >> - fix error handling while in the middle of transmitting GSO skbs
> >>
> >> I've also audited the tree once again for invalid return values and
> >> found a single remaining instance in a Wimax driver, I'll take care
> >> of that later.
> >>
> >> Two questions remain:
> >>
> >> - 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.)

> 
> >> diff --git a/net/core/dev.c b/net/core/dev.c
> >> index bf629ac..1f5752d 100644
> >> --- a/net/core/dev.c
> >> +++ b/net/core/dev.c
> >> @@ -1756,7 +1756,7 @@ int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
> >>  			struct netdev_queue *txq)
> >>  {
> >>  	const struct net_device_ops *ops = dev->netdev_ops;
> >> -	int rc;
> >> +	int rc = NETDEV_TX_OK;
> > 
> > Isn't it enough to add this in one place only: before this
> > "goto out_kfree_skb"?
> 
> Its only exists once in the version I sent out earlier.
> 
> >>  	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.

> 
> >>  			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?

Jarek P.