Re: [Patch V2 1/4] iommu/vt-d: replace *hdr with hdr[0] in struct dmar_drhd_unit
On Mon, Mar 21, 2016 at 05:53:15PM +0100, Thomas Gleixner wrote: >On Mon, 21 Mar 2016, Wei Yang wrote: >> On Sun, Mar 20, 2016 at 04:42:29PM +0100, Thomas Gleixner wrote: >> >On Sun, 20 Mar 2016, Wei Yang wrote: >> > >> >> hdr in struct dmar_drhd_unit is used to point the DMAR hardware unit >> >> copied >> >> at the end of struct dmar_drhd_unit. One zero-sized array may be more >> >> elegant for this purpose. >> > >> >You forget to tell why. >> > >> >> One possible benefit is to save some space. >> >> Before commit 6b1972493a84 "iommu/vt-d: Implement DMAR unit hotplug >> framework", >> dmaru->hdr just points to the memory region of DMA remapping hardware >> definition. In this case, it would have no difference to where we put hdr. >> >> After this commit, DMA remapping hardware definition is copied and >> attach to the end of dmaru structure. By replacing a pointer with a >> zero-sized >> array, that would save some space for this structure. > >Sure and exactly that explanation should be in the changelog. Not some >handwaving "may be more elegant". We don't care about elegance, we care about >correctness. > Hey, Thomas Nice to see your comment again~ You are right, I would add this in the changelog. >> >> This patch replace *hdr with hdr[0] in struct dmar_drhd_unit. >> >> >> >> Besides this, this patch includes other two changes: >> >> 1. remove unnecessary type cast in dmar_table_detect() >> > >> >Again. Why is it not necessary? >> > >> >> The second parameter's type of function acpi_get_table_with_size() is "struct >> acpi_table_header **", and type of dmar_tbl is "struct acpi_table_header *". >> >> So without the type cast, the type of parameter and the function definition >> matches. > >That's the information which a changelog wants to have, because otherwise a >reviewer is forced to lookup the prototypes > >So a simple: > > "Remove redundant type case to same type" > >would have told me what you are doing. > Agree, thanks for your suggestion, this really helps reviewers. >> >> 2. type cast from acpi_dmar_header to acpi_dmar_hardware_unit directly >> > >> >Don't even think about doing that. container_of() is there for a reason. >> > >> >Your change works today, because the embedded structure is the first one in >> >the containing structure. If the containing structure gets reordered later, >> >the whole thing will explode in hard to debug ways. >> > >> >Even if such a reordering is unlikely in that ACPI case, we just don't do >> >that. It's bad and sloppy coding style. The generated code is the same. >> > >> >> Yes, I agree with you that make this change should be very careful, while I >> think the original usage of container_of() is not necessary. > >It's not necessary, but it is correct. Removing it is just putting assumptions >into the code, which is never a good idea. > >> Literally, it converts "struct acpi_dmar_header" to "struct >> acpi_dmar_hardware_unit", because the first one is an element "header" of the >> second one. While let's look at how the dmaru->hdr is initialized in >> dmar_parse_one_drhd(), we copy the memory of "struct acpi_dmar_hardware_unit" >> to a region where dmaru->hdr points to. So the code itself implies "struct >> acpi_dmar_header" is the first element of "struct acpi_dmar_hardware_unit". >u link >Which is wrong to begin with. Assumption of first elements are crap. > >> Otherwise, we can't do this memcpy in dmar_parse_one_drhd(). > >> BTW, I am thinking changing the type of dmaru->hdr from "struct >> acpi_dmar_header *" to "struct acpi_dmar_hardware_unit *". By doing so the >> code would be more self explain, and it doesn't need to cast back and forth. > >Yes, that makes sense. > Ah, I am happy that you like this idea :-) Let me format V3. Have a good night:) >Thanks, > > tglx -- Wei Yang Help you, Help me ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [Patch V2 1/4] iommu/vt-d: replace *hdr with hdr[0] in struct dmar_drhd_unit
On Mon, 21 Mar 2016, Wei Yang wrote: > On Sun, Mar 20, 2016 at 04:42:29PM +0100, Thomas Gleixner wrote: > >On Sun, 20 Mar 2016, Wei Yang wrote: > > > >> hdr in struct dmar_drhd_unit is used to point the DMAR hardware unit copied > >> at the end of struct dmar_drhd_unit. One zero-sized array may be more > >> elegant for this purpose. > > > >You forget to tell why. > > > > One possible benefit is to save some space. > > Before commit 6b1972493a84 "iommu/vt-d: Implement DMAR unit hotplug > framework", > dmaru->hdr just points to the memory region of DMA remapping hardware > definition. In this case, it would have no difference to where we put hdr. > > After this commit, DMA remapping hardware definition is copied and > attach to the end of dmaru structure. By replacing a pointer with a zero-sized > array, that would save some space for this structure. Sure and exactly that explanation should be in the changelog. Not some handwaving "may be more elegant". We don't care about elegance, we care about correctness. > >> This patch replace *hdr with hdr[0] in struct dmar_drhd_unit. > >> > >> Besides this, this patch includes other two changes: > >> 1. remove unnecessary type cast in dmar_table_detect() > > > >Again. Why is it not necessary? > > > > The second parameter's type of function acpi_get_table_with_size() is "struct > acpi_table_header **", and type of dmar_tbl is "struct acpi_table_header *". > > So without the type cast, the type of parameter and the function definition > matches. That's the information which a changelog wants to have, because otherwise a reviewer is forced to lookup the prototypes So a simple: "Remove redundant type case to same type" would have told me what you are doing. > >> 2. type cast from acpi_dmar_header to acpi_dmar_hardware_unit directly > > > >Don't even think about doing that. container_of() is there for a reason. > > > >Your change works today, because the embedded structure is the first one in > >the containing structure. If the containing structure gets reordered later, > >the whole thing will explode in hard to debug ways. > > > >Even if such a reordering is unlikely in that ACPI case, we just don't do > >that. It's bad and sloppy coding style. The generated code is the same. > > > > Yes, I agree with you that make this change should be very careful, while I > think the original usage of container_of() is not necessary. It's not necessary, but it is correct. Removing it is just putting assumptions into the code, which is never a good idea. > Literally, it converts "struct acpi_dmar_header" to "struct > acpi_dmar_hardware_unit", because the first one is an element "header" of the > second one. While let's look at how the dmaru->hdr is initialized in > dmar_parse_one_drhd(), we copy the memory of "struct acpi_dmar_hardware_unit" > to a region where dmaru->hdr points to. So the code itself implies "struct > acpi_dmar_header" is the first element of "struct acpi_dmar_hardware_unit". Which is wrong to begin with. Assumption of first elements are crap. > Otherwise, we can't do this memcpy in dmar_parse_one_drhd(). > BTW, I am thinking changing the type of dmaru->hdr from "struct > acpi_dmar_header *" to "struct acpi_dmar_hardware_unit *". By doing so the > code would be more self explain, and it doesn't need to cast back and forth. Yes, that makes sense. Thanks, tglx ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [Patch V2 1/4] iommu/vt-d: replace *hdr with hdr[0] in struct dmar_drhd_unit
Hi, Thomas Glad to receive your comment. Please see my reply below. On Sun, Mar 20, 2016 at 04:42:29PM +0100, Thomas Gleixner wrote: >On Sun, 20 Mar 2016, Wei Yang wrote: > >> hdr in struct dmar_drhd_unit is used to point the DMAR hardware unit copied >> at the end of struct dmar_drhd_unit. One zero-sized array may be more >> elegant for this purpose. > >You forget to tell why. > One possible benefit is to save some space. Before commit 6b1972493a84 "iommu/vt-d: Implement DMAR unit hotplug framework", dmaru->hdr just points to the memory region of DMA remapping hardware definition. In this case, it would have no difference to where we put hdr. After this commit, DMA remapping hardware definition is copied and attach to the end of dmaru structure. By replacing a pointer with a zero-sized array, that would save some space for this structure. >> This patch replace *hdr with hdr[0] in struct dmar_drhd_unit. >> >> Besides this, this patch includes other two changes: >> 1. remove unnecessary type cast in dmar_table_detect() > >Again. Why is it not necessary? > The second parameter's type of function acpi_get_table_with_size() is "struct acpi_table_header **", and type of dmar_tbl is "struct acpi_table_header *". So without the type cast, the type of parameter and the function definition matches. That's why I want to remove it. >> 2. type cast from acpi_dmar_header to acpi_dmar_hardware_unit directly > >Don't even think about doing that. container_of() is there for a reason. > >Your change works today, because the embedded structure is the first one in >the containing structure. If the containing structure gets reordered later, >the whole thing will explode in hard to debug ways. > >Even if such a reordering is unlikely in that ACPI case, we just don't do >that. It's bad and sloppy coding style. The generated code is the same. > Yes, I agree with you that make this change should be very careful, while I think the original usage of container_of() is not necessary. Literally, it converts "struct acpi_dmar_header" to "struct acpi_dmar_hardware_unit", because the first one is an element "header" of the second one. While let's look at how the dmaru->hdr is initialized in dmar_parse_one_drhd(), we copy the memory of "struct acpi_dmar_hardware_unit" to a region where dmaru->hdr points to. So the code itself implies "struct acpi_dmar_header" is the first element of "struct acpi_dmar_hardware_unit". Otherwise, we can't do this memcpy in dmar_parse_one_drhd(). BTW, I am thinking changing the type of dmaru->hdr from "struct acpi_dmar_header *" to "struct acpi_dmar_hardware_unit *". By doing so the code would be more self explain, and it doesn't need to cast back and forth. Thanks again for your review, have a good day~ >Thanks, > > tglx -- Wei Yang Help you, Help me ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [Patch V2 1/4] iommu/vt-d: replace *hdr with hdr[0] in struct dmar_drhd_unit
On Sun, 20 Mar 2016, Wei Yang wrote: > hdr in struct dmar_drhd_unit is used to point the DMAR hardware unit copied > at the end of struct dmar_drhd_unit. One zero-sized array may be more > elegant for this purpose. You forget to tell why. > This patch replace *hdr with hdr[0] in struct dmar_drhd_unit. > > Besides this, this patch includes other two changes: > 1. remove unnecessary type cast in dmar_table_detect() Again. Why is it not necessary? > 2. type cast from acpi_dmar_header to acpi_dmar_hardware_unit directly Don't even think about doing that. container_of() is there for a reason. Your change works today, because the embedded structure is the first one in the containing structure. If the containing structure gets reordered later, the whole thing will explode in hard to debug ways. Even if such a reordering is unlikely in that ACPI case, we just don't do that. It's bad and sloppy coding style. The generated code is the same. Thanks, tglx ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
