Masa Murayama wrote: > Steven, > > Thank you very much for your review. > I answer in lines.
My pleasure; I am glad I could help. >> (the changes on lines 1140 - 1180 really bug me): >> >> 1146 tmp0 is not meaningful - why was the assignment broken out? IMHO >> having this broken apart makes the code more difficult to read. >> 1147 tmp1 is not meaningful - see comments above (line 1146) >> 1177 this line should not have changed; using a tmp variable here has >> no benefit >> 1178 I would have handled this differently. On line 1170, I would >> have: 'mark |= dcp->dmac_size;', and then >> 'tdp->d_cmdsts = LE_32(mark);' on line 1180. > > The reason of changing line 1140 - 1180 is for performance. > As LE_32() is a macro, LE_32(p->xxx) will cause several memory > references to the same location by a pointer, that are difficult for > compilers to optimize. This is very interesting - I honestly would not have considered dereferencing a pointer from within a macro to be a performance issue - now that you have pointed it out, I can understand why the assignments were broken apart. >> 1243 See comments on line 1188 >> >> 1256 & 1257 Don't break the statements apart; clutter > > The reason is for supporting big endian CPUs, i.e. sparc. > LE_32(tdp->d_cmdsts) is not atomic in sparc. LE_32() macro > generates several memory references to read the same word in the same > descriptor. > It will corrupt the result of LE_32() if the nic will update the > descriptor at the same time. Makes sense, I did not even consider the side-effects of the inline. (Minor rant...) It's cases like the ones above that really make me wonder if LE/BE_32 and friends should be amended. It seems that the behavior would be far more consistent if LE_32 on a BE system would simply call a function rather than attempt to convert the value inline. Pass-by-value would insulate the caller from any non-atomic side-effects of the inline (at the additional cost of performing a function call). Then again, I suppose it depends on what is considered acceptable overhead when converting from LE to BE (and vice versa) and how often it is performed. Regards, Steve -- Yet magic and hierarchy arise from the same source, and this source has a null pointer. Reference the NULL within NULL, it is the gateway to all wizardry. _______________________________________________ driver-discuss mailing list driver-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/driver-discuss
