Re: [PATCH] Intel-IOMMU: Delete an error message for a failed memory allocation in init_dmars()

[Joe Perches]

On Sun, 2018-01-21 at 08:19 +0100, Jörg Rödel wrote:
> On Sat, Jan 20, 2018 at 05:37:52PM -0800, Joe Perches wrote:
> > While Markus' commit messages are nearly universally terrible,
> > is there really any signficant value in knowing when any
> > particular OOM condition occurs other than the subsystem that
> > became OOM?
> > 
> > You're going to be hosed in any case.
> > 
> > Why isn't the generic OOM stack dump good enough?
> 
> Because if we know the exact allocation attempt that failed right away,
> we can more easily check if we can rewrite it so that it is more likely
> to succeed, e.g. rewriting one higher-order allocation to multiple
> order-0 allocations.

Up to you but I think that's pretty dubious
as this is an init function and if it fails
the system really is stuffed.

Unrelated, there are some unnecessary casts
of pointers to void * that could be removed
to help make the code a bit more regular as
some callers use the cast and some do not.

---
 drivers/iommu/intel-iommu.c | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 4a2de34895ec..8d7ea76345ae 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -845,8 +845,8 @@ static inline struct context_entry 
*iommu_context_addr(struct intel_iommu *iommu
if (!context)
return NULL;
 
-   __iommu_flush_cache(iommu, (void *)context, CONTEXT_SIZE);
-   phy_addr = virt_to_phys((void *)context);
+   __iommu_flush_cache(iommu, context, CONTEXT_SIZE);
+   phy_addr = virt_to_phys(context);
*entry = phy_addr | 1;
__iommu_flush_cache(iommu, entry, sizeof(*entry));
}
@@ -1298,7 +1298,7 @@ static int iommu_alloc_root_entry(struct intel_iommu 
*iommu)
struct root_entry *root;
unsigned long flags;
 
-   root = (struct root_entry *)alloc_pgtable_page(iommu->node);
+   root = alloc_pgtable_page(iommu->node);
if (!root) {
pr_err("Allocating root entry for %s failed\n",
iommu->name);
@@ -1978,7 +1978,7 @@ static int domain_init(struct dmar_domain *domain, struct 
intel_iommu *iommu,
domain->nid = iommu->node;
 
/* always allocate the top pgd */
-   domain->pgd = (struct dma_pte *)alloc_pgtable_page(domain->nid);
+   domain->pgd = alloc_pgtable_page(domain->nid);
if (!domain->pgd)
return -ENOMEM;
__iommu_flush_cache(iommu, domain->pgd, PAGE_SIZE);
@@ -4168,7 +4168,7 @@ int __init dmar_parse_one_rmrr(struct acpi_dmar_header 
*header, void *arg)
if (!rmrru->resv)
goto free_rmrru;
 
-   rmrru->devices = dmar_alloc_dev_scope((void *)(rmrr + 1),
+   rmrru->devices = dmar_alloc_dev_scope(rmrr + 1,
((void *)rmrr) + rmrr->header.length,
>devices_cnt);
if (rmrru->devices_cnt && rmrru->devices == NULL)
@@ -4229,7 +4229,7 @@ int dmar_parse_one_atsr(struct acpi_dmar_header *hdr, 
void *arg)
memcpy(atsru->hdr, hdr, hdr->length);
atsru->include_all = atsr->flags & 0x1;
if (!atsru->include_all) {
-   atsru->devices = dmar_alloc_dev_scope((void *)(atsr + 1),
+   atsru->devices = dmar_alloc_dev_scope(atsr + 1,
(void *)atsr + atsr->header.length,
>devices_cnt);
if (atsru->devices_cnt && atsru->devices == NULL) {
@@ -4465,7 +4465,7 @@ int dmar_iommu_notify_scope_dev(struct 
dmar_pci_notify_info *info)
rmrr = container_of(rmrru->hdr,
struct acpi_dmar_reserved_memory, header);
if (info->event == BUS_NOTIFY_ADD_DEVICE) {
-   ret = dmar_insert_dev_scope(info, (void *)(rmrr + 1),
+   ret = dmar_insert_dev_scope(info, rmrr + 1,
((void *)rmrr) + rmrr->header.length,
rmrr->segment, rmrru->devices,
rmrru->devices_cnt);
@@ -4483,7 +4483,7 @@ int dmar_iommu_notify_scope_dev(struct 
dmar_pci_notify_info *info)
 
atsr = container_of(atsru->hdr, struct acpi_dmar_atsr, header);
if (info->event == BUS_NOTIFY_ADD_DEVICE) {
-   ret = dmar_insert_dev_scope(info, (void *)(atsr + 1),
+   ret = dmar_insert_dev_scope(info, atsr + 1,
(void *)atsr + atsr->header.length,
atsr->segment, atsru->devices,
atsru->devices_cnt);
@@ -4920,7 +4920,7 @@ static int md_domain_init(struct dmar_domain *domain, int 
guest_width)
domain->max_addr = 0;
 
/* always allocate the top pgd */
-   domain->pgd = (struct 

Re: [PATCH] Intel-IOMMU: Delete an error message for a failed memory allocation in init_dmars()

[Joe Perches]

On Sat, 2018-01-20 at 20:40 +0100, Jörg Rödel wrote:
> On Sat, Jan 20, 2018 at 03:55:37PM +0100, SF Markus Elfring wrote:
> > Do you need any more background information for this general
> > transformation pattern?
> 
> No.
> 
> > Do you find the Linux allocation failure report insufficient for this
> > use case?
> 
> Yes, because it can't tell me what the code was trying to allocate.

While Markus' commit messages are nearly universally terrible,
is there really any signficant value in knowing when any
particular OOM condition occurs other than the subsystem that
became OOM?

You're going to be hosed in any case.

Why isn't the generic OOM stack dump good enough?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] Intel-IOMMU: Delete an error message for a failed memory allocation in init_dmars()

[Jörg Rödel]

On Sat, Jan 20, 2018 at 05:37:52PM -0800, Joe Perches wrote:
> While Markus' commit messages are nearly universally terrible,
> is there really any signficant value in knowing when any
> particular OOM condition occurs other than the subsystem that
> became OOM?
> 
> You're going to be hosed in any case.
> 
> Why isn't the generic OOM stack dump good enough?

Because if we know the exact allocation attempt that failed right away,
we can more easily check if we can rewrite it so that it is more likely
to succeed, e.g. rewriting one higher-order allocation to multiple
order-0 allocations.


Joerg

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] Intel-IOMMU: Delete an error message for a failed memory allocation in init_dmars()

[Jörg Rödel]

On Sat, Jan 20, 2018 at 03:55:37PM +0100, SF Markus Elfring wrote:
> Do you need any more background information for this general
> transformation pattern?

No.

> Do you find the Linux allocation failure report insufficient for this
> use case?

Yes, because it can't tell me what the code was trying to allocate.

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] Intel-IOMMU: Delete an error message for a failed memory allocation in init_dmars()

[SF Markus Elfring]

>> Date: Sat, 20 Jan 2018 13:51:49 +0100
>>
>> Omit an extra message for a memory allocation failure in this function.
>>
>> This issue was detected by using the Coccinelle software.
>>
>> Signed-off-by: Markus Elfring 
> 
> NACK on this one and the other two IOMMU patches you sent.

Do you need any more background information for this general transformation 
pattern?


> The messages give the user/developer useful information about what the memory
> that failed to allocate would have been used for.

Do you find the Linux allocation failure report insufficient for this use case?

Regards,
Markus
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] Intel-IOMMU: Delete an error message for a failed memory allocation in init_dmars()

[Jörg Rödel]

On Sat, Jan 20, 2018 at 02:00:13PM +0100, SF Markus Elfring wrote:
> From: Markus Elfring 
> Date: Sat, 20 Jan 2018 13:51:49 +0100
> 
> Omit an extra message for a memory allocation failure in this function.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring 

NACK on this one and the other two IOMMU patches you sent. The messages
give the user/developer useful information about what the memory that
failed to allocate would have been used for.


Joerg
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH] Intel-IOMMU: Delete an error message for a failed memory allocation in init_dmars()

[SF Markus Elfring]

From: Markus Elfring 
Date: Sat, 20 Jan 2018 13:51:49 +0100

Omit an extra message for a memory allocation failure in this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
 drivers/iommu/intel-iommu.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 4a2de34895ec..f4ba6bdf7863 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -3258,7 +3258,6 @@ static int __init init_dmars(void)
g_iommus = kcalloc(g_num_of_iommus, sizeof(struct intel_iommu *),
GFP_KERNEL);
if (!g_iommus) {
-   pr_err("Allocating global iommu array failed\n");
ret = -ENOMEM;
goto error;
}
-- 
2.15.1

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu