| Message ID | 20210726174010.396765-1-elder@linaro.org |
|---|---|
| Headers | show |
| Series | net: ipa: kill IPA_VALIDATION | expand |
Hello: This series was applied to netdev/net-next.git (refs/heads/master): On Mon, 26 Jul 2021 12:40:06 -0500 you wrote: > A few months ago I proposed cleaning up some code that validates > certain things conditionally, arguing that doing so once is enough, > thus doing so always should not be necessary. > https://lore.kernel.org/netdev/20210320141729.1956732-1-elder@linaro.org/ > Leon Romanovsky felt strongly that this was a mistake, and in the > end I agreed to change my plans. > > [...] Here is the summary with links: - [net-next,1/4] net: ipa: fix ipa_cmd_table_valid() https://git.kernel.org/netdev/net-next/c/f2c1dac0abcf - [net-next,2/4] net: ipa: always validate filter and route tables https://git.kernel.org/netdev/net-next/c/546948bf3625 - [net-next,3/4] net: ipa: kill the remaining conditional validation code https://git.kernel.org/netdev/net-next/c/442d68ebf092 - [net-next,4/4] net: ipa: use WARN_ON() rather than assertions https://git.kernel.org/netdev/net-next/c/5bc5588466a1 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
On Mon, Jul 26, 2021 at 12:40:06PM -0500, Alex Elder wrote: > A few months ago I proposed cleaning up some code that validates > certain things conditionally, arguing that doing so once is enough, > thus doing so always should not be necessary. > https://lore.kernel.org/netdev/20210320141729.1956732-1-elder@linaro.org/ > Leon Romanovsky felt strongly that this was a mistake, and in the > end I agreed to change my plans. <...> > The second patch fixes a bug that wasn't normally exposed because of > the conditional compilation (a reason Leon was right about this). Thanks Alex, If you want another anti pattern that is very popular in netdev, the following pattern is wrong by definition :): if (WARN_ON(...)) return ... The WARN_*() macros are intended catch impossible flows, something that shouldn't exist. The idea that printed stack to dmesg and return to the caller will fix the situation is a very naive one. That stack already says that something very wrong in the system. If such flow can be valid use "if(...) return ..", if not use plain WARN_ON(...). Thanks
On 7/27/21 6:16 AM, Leon Romanovsky wrote: > On Mon, Jul 26, 2021 at 12:40:06PM -0500, Alex Elder wrote: >> A few months ago I proposed cleaning up some code that validates >> certain things conditionally, arguing that doing so once is enough, >> thus doing so always should not be necessary. >> https://lore.kernel.org/netdev/20210320141729.1956732-1-elder@linaro.org/ >> Leon Romanovsky felt strongly that this was a mistake, and in the >> end I agreed to change my plans. > > <...> > >> The second patch fixes a bug that wasn't normally exposed because of >> the conditional compilation (a reason Leon was right about this). > > Thanks Alex, > > If you want another anti pattern that is very popular in netdev, the following pattern is > wrong by definition :): > if (WARN_ON(...)) > return ... I understand this reasoning. I had it return an error if the WARN_ON() condition was true in cases where the function returned a value and callers already handled errors. I looked back at the patch and here is one of those cases: gsi_channel_trans_alloc() - If too many TREs are requested we do not want to allocate them from the pool, or it will cause further breakage. By returning early, no transaction will be filled or committed, and an error message will (often) be reported, which will indicate the source of the error. If any error occurs during initialization, we fail that whole process and everything should be cleaned up. So in this case at least, returning if this ever occurred is better than allowing control to continue into the function. In any case I take your point. I will now add to my task list a review of these spots. I'd like to be sure an error message *is* reported at an appropriate level up the chain of callers so I can always identify the culprit in the a WARN_ON() fires (even though it should never happen). And in each case I'll evaluate whether returning is better than not. Thanks. -Alex > The WARN_*() macros are intended catch impossible flows, something that > shouldn't exist. The idea that printed stack to dmesg and return to the > caller will fix the situation is a very naive one. That stack already > says that something very wrong in the system. > > If such flow can be valid use "if(...) return ..", if not use plain > WARN_ON(...). > > Thanks >
On Tue, Jul 27, 2021 at 07:34:41AM -0500, Alex Elder wrote: > On 7/27/21 6:16 AM, Leon Romanovsky wrote: > > On Mon, Jul 26, 2021 at 12:40:06PM -0500, Alex Elder wrote: > >> A few months ago I proposed cleaning up some code that validates > >> certain things conditionally, arguing that doing so once is enough, > >> thus doing so always should not be necessary. > >> https://lore.kernel.org/netdev/20210320141729.1956732-1-elder@linaro.org/ > >> Leon Romanovsky felt strongly that this was a mistake, and in the > >> end I agreed to change my plans. > > > > <...> > > > >> The second patch fixes a bug that wasn't normally exposed because of > >> the conditional compilation (a reason Leon was right about this). > > > > Thanks Alex, > > > > If you want another anti pattern that is very popular in netdev, the following pattern is > > wrong by definition :): > > if (WARN_ON(...)) > > return ... > > I understand this reasoning. > > I had it return an error if the WARN_ON() condition was true in cases > where the function returned a value and callers already handled errors. > I looked back at the patch and here is one of those cases: > > gsi_channel_trans_alloc() > - If too many TREs are requested we do not want to allocate them > from the pool, or it will cause further breakage. By returning > early, no transaction will be filled or committed, and an error > message will (often) be reported, which will indicate the source > of the error. If any error occurs during initialization, we fail > that whole process and everything should be cleaned up. So in > this case at least, returning if this ever occurred is better > than allowing control to continue into the function. > > In any case I take your point. I will now add to my task list > a review of these spots. I'd like to be sure an error message > *is* reported at an appropriate level up the chain of callers so > I can always identify the culprit in the a WARN_ON() fires (even > though it should never > happen). And in each case I'll evaluate > whether returning is better than not. You can, but users don't :). So if it is valid but error flow, that needs user awareness, simply print something to the dmesg with *_err() prints. BTW, I'm trying to untangle some of the flows in net/core/devlink.c and such if(WARN()) pattern is even harmful, because it is very hard to understand when that error is rare/non-exist/real. Thanks > > Thanks. > > -Alex > > > The WARN_*() macros are intended catch impossible flows, something that > > shouldn't exist. The idea that printed stack to dmesg and return to the > > caller will fix the situation is a very naive one. That stack already > > says that something very wrong in the system. > > > > If such flow can be valid use "if(...) return ..", if not use plain > > WARN_ON(...). > > > > Thanks > > >
On 7/27/21 7:56 AM, Leon Romanovsky wrote: >> In any case I take your point. I will now add to my task list >> a review of these spots. I'd like to be sure an error message >> *is* reported at an appropriate level up the chain of callers so >> I can always identify the culprit in the a WARN_ON() fires (even >> though it should never >> happen). And in each case I'll evaluate >> whether returning is better than not. > You can, but users don't :). So if it is valid but error flow, that > needs user awareness, simply print something to the dmesg with *_err() > prints. For some reason you seem to care about users. I guess the WARN stack trace tells me where it comes from. This would be an invalid error flow, and should never happen. I'll still plan to review each of these again. > BTW, I'm trying to untangle some of the flows in net/core/devlink.c > and such if(WARN()) pattern is even harmful, because it is very hard to > understand when that error is rare/non-exist/real. That's what assert() is for, but we've already had that discussion :) -Alex
On Tue, Jul 27, 2021 at 08:40:42AM -0500, Alex Elder wrote: > On 7/27/21 7:56 AM, Leon Romanovsky wrote: > > > In any case I take your point. I will now add to my task list > > > a review of these spots. I'd like to be sure an error message > > > *is* reported at an appropriate level up the chain of callers so > > > I can always identify the culprit in the a WARN_ON() fires (even > > > though it should never > > > happen). And in each case I'll evaluate > > > whether returning is better than not. > > You can, but users don't :). So if it is valid but error flow, that > > needs user awareness, simply print something to the dmesg with *_err() > > prints. > > For some reason you seem to care about users. Yeah, this is my Achilles heel :) Thanks