| Message ID | 1430148038-25695-1-git-send-email-mschmidt@redhat.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Michal Schmidt <mschmidt@redhat.com> Date: Mon, 27 Apr 2015 17:20:38 +0200 > bnx2x's 'disable_tpa=1' module option is not respected properly and TPA > (transparent packet aggregation) remains enabled. Even though the > module option causes LRO to be disabled, TPA is enabled in GRO mode. > > Additionally, disabling GRO via ethtool then has no effect. One can > still observe tpa_* statistics increase and large packets being received > in tcpdump. > > The bug was an unintended consequence of commit aebf6244cd39 "bnx2x: Be > more forgiving toward SW GRO". > > Fix it by following the bp->disable_tpa flag when initializing fp's. > > Signed-off-by: Michal Schmidt <mschmidt@redhat.com> Applied, thanks. -- 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
>> bnx2x's 'disable_tpa=1' module option is not respected properly and TPA >> (transparent packet aggregation) remains enabled. Even though the >> module option causes LRO to be disabled, TPA is enabled in GRO mode. >> >> Additionally, disabling GRO via ethtool then has no effect. One can >> still observe tpa_* statistics increase and large packets being received >> in tcpdump. >> >> The bug was an unintended consequence of commit aebf6244cd39 "bnx2x: Be >> more forgiving toward SW GRO". >> >> Fix it by following the bp->disable_tpa flag when initializing fp's. >> >> Signed-off-by: Michal Schmidt <mschmidt@redhat.com> > >Applied, thanks. Michal - thanks, but Dave -is it possible you gave us only 2.5 hours to review this one? I agree that this fixes an incorrect logic, but its incomplete as the bnx2x 'bp->flags' no longer represent the correct logic. I.e., it might cause additional issues down the road, as the 'fp->mode' and 'fp->disable_tpa' are no longer in sync. We already have a fix for this issue, we just haven't sent it to 'net' yet. Are you willing to give us an additional day to send an alternative fix? Or do you prefer to take this, and possibly later we'll remove the logic added by this patch in our own? -- 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
From: Yuval Mintz <Yuval.Mintz@qlogic.com> Date: Mon, 27 Apr 2015 18:15:44 +0000 >>> bnx2x's 'disable_tpa=1' module option is not respected properly and TPA >>> (transparent packet aggregation) remains enabled. Even though the >>> module option causes LRO to be disabled, TPA is enabled in GRO mode. >>> >>> Additionally, disabling GRO via ethtool then has no effect. One can >>> still observe tpa_* statistics increase and large packets being received >>> in tcpdump. >>> >>> The bug was an unintended consequence of commit aebf6244cd39 "bnx2x: Be >>> more forgiving toward SW GRO". >>> >>> Fix it by following the bp->disable_tpa flag when initializing fp's. >>> >>> Signed-off-by: Michal Schmidt <mschmidt@redhat.com> >> >>Applied, thanks. > > Michal - thanks, but Dave -is it possible you gave us only 2.5 hours to > review this one? You'll need to send me a revert or changes relative to this one as the commit is not only permanent when it reaches my tree, it's also made it's way into Linus's tree now as well. -- 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 04/27/2015 08:15 PM, Yuval Mintz wrote: > I agree that this fixes an incorrect logic, but its incomplete as the bnx2x > 'bp->flags' no longer represent the correct logic. I.e., it might cause > additional issues down the road, as the 'fp->mode' and 'fp->disable_tpa' > are no longer in sync. > We already have a fix for this issue, we just haven't sent it to 'net' > yet. Are you willing to give us an additional day to send an > alternative fix? Or do you prefer to take this, and possibly later > we'll remove the logic added by this patch in our own? Yuval, I also have a patch that removes fp->disable_tpa by merging it into fp->mode (adding a 3rd mode: TPA_MODE_DISABLED). And another patch that removes {TPA,GRO}_ENABLE_FLAG by using dev->features everywhere. I'll wait and see what changes you have. Regards, Michal -- 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
> > I agree that this fixes an incorrect logic, but its incomplete as the > > bnx2x 'bp->flags' no longer represent the correct logic. I.e., it > > might cause additional issues down the road, as the 'fp->mode' and 'fp- > >disable_tpa' > > are no longer in sync. > > > We already have a fix for this issue, we just haven't sent it to 'net' > > yet. Are you willing to give us an additional day to send an > > alternative fix? Or do you prefer to take this, and possibly later > > we'll remove the logic added by this patch in our own? > > Yuval, > > I also have a patch that removes fp->disable_tpa by merging it into > fp->mode (adding a 3rd mode: TPA_MODE_DISABLED). And another patch that > removes {TPA,GRO}_ENABLE_FLAG by using dev->features everywhere. > I'll wait and see what changes you have. > > Regards, > Michal Ours is merely a point fix that differentiate GRO_ENABLE_FLAG from NETIF_F_GRO. If you have a ready patch that removes the flags altogether I think we can skip ours and review that one instead. Thanks, Yuval -- 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/ethernet/broadcom/bnx2x/bnx2x_cmn.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c index 6f7dc81581..3558a36b1c 100644 --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c @@ -2485,8 +2485,10 @@ static void bnx2x_bz_fp(struct bnx2x *bp, int index) else if (bp->flags & GRO_ENABLE_FLAG) fp->mode = TPA_MODE_GRO; - /* We don't want TPA on an FCoE L2 ring */ - if (IS_FCOE_FP(fp)) + /* We don't want TPA if it's disabled in bp + * or if this is an FCoE L2 ring. + */ + if (bp->disable_tpa || IS_FCOE_FP(fp)) fp->disable_tpa = 1; }
bnx2x's 'disable_tpa=1' module option is not respected properly and TPA (transparent packet aggregation) remains enabled. Even though the module option causes LRO to be disabled, TPA is enabled in GRO mode. Additionally, disabling GRO via ethtool then has no effect. One can still observe tpa_* statistics increase and large packets being received in tcpdump. The bug was an unintended consequence of commit aebf6244cd39 "bnx2x: Be more forgiving toward SW GRO". Fix it by following the bp->disable_tpa flag when initializing fp's. Signed-off-by: Michal Schmidt <mschmidt@redhat.com> --- drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)