RE: [PATCH] qede: fix CONFIG_INFINIBAND_QEDR=m build error
From: Mintz, Yuval <hidden>
Date: 2016-10-13 10:44:43
Also in:
lkml
quoted
quoted
quoted
While I don't mind, you could have argued is that we're not removing enough, not too much. I.e., perhaps the rdma_msix_* fields should also have been ifdef-ed instead. [in which case this solution would not have worked]That would add even more #ifdefs though.I agree. Although I'm never clear on the guidelines for the tradeoff - How much memory/code is considered too much so that you'd have To ifdef code out instead of 'wasting'? [I obviously don't claim 64 bytes of memory hit that threshold]I don't think code size should ever be a reason for an #ifdef in a .c file: if the code is well-structured, you can always get the same object code using if(IS_ENABLED()) checks within the code at better readability or better compile-time coverage. Between if(IS_ENABLED()) checks and inline helpers, it usually doesn't matter much either way as long as the separation between the modules is clear enough. In the example above, removing the structure fields however would require to move the debugging output into another inline function though.
Still, the question remains - If we were to allocate X bytes of memory per-something [in this case, per qed owned PCI function], and that memory wouldn't be functional without a some CONFIG option enabled, how big should X become before we'd decide the fields should also be dependent on the option? It bears no real relevance to this case, as the fields involved are insignificantly small. But still - is there a rule of thumb here?
quoted
BTW, are you interested in doing a v2 for this? Or would you prefer if we'd pick it up from here?I think it's better if you do a v2, as you better understand the long-term plans. I'd be happy to test your patch in my randconfig build setup if you like.
Sure.