Re: FW: [PATCH 13/17] mlx4: Add blue flame support for kernel consumers

Re: FW: [PATCH 13/17] mlx4: Add blue flame support for kernel consumers

[Eli Cohen]

> 
> On Mon, Mar 7, 2011 at 2:17 AM, Yevgeny Petrilin
> <yevgenyp@mellanox.co.il> wrote:
> > +       if (map_bf_area(dev))
> > +               mlx4_dbg(dev, "Kernel support for blue flame is not available for kernels < 2.6.28\n");
> 
> This seems like a really bad error message.  Can map_bf_area() actually fail?

I agree that this message is inappropriate here; it is originiated
from the OFED patches which support older kernels too.
As for the question if a message is justified here at all, I think the
answer is yes becuase of this:

+static int map_bf_area(struct mlx4_dev *dev)
+{
+       struct mlx4_priv *priv = mlx4_priv(dev);
+       resource_size_t bf_start;
+       resource_size_t bf_len;
+       int err = 0;
+
+       bf_start = pci_resource_start(dev->pdev, 2) +
(dev->caps.num_uars << PAGE_SHIFT);
+       bf_len = pci_resource_len(dev->pdev, 2) - (dev->caps.num_uars
<< PAGE_SHIFT);
+       priv->bf_mapping = io_mapping_create_wc(bf_start, bf_len);
+       if (!priv->bf_mapping)
+               err = -ENOMEM;

Specifically, some archs may not support write combining.
 

Re: [PATCH 13/17] mlx4: Add blue flame support for kernel consumers

[David Miller]

From: Eli Cohen <eli@dev.mellanox.co.il>
Date: Mon, 7 Mar 2011 23:36:48 +0200

> Specifically, some archs may not support write combining.

They should just create a non-write-combining mapping if they
don't support it.

It could still fail due to resource constraints, but not because
of the reason you're stating.

Re: [PATCH 13/17] mlx4: Add blue flame support for kernel consumers

[Eli Cohen]

On Mon, Mar 07, 2011 at 01:40:01PM -0800, David Miller wrote:
> From: Eli Cohen <eli@dev.mellanox.co.il>
> Date: Mon, 7 Mar 2011 23:36:48 +0200
> 
> > Specifically, some archs may not support write combining.
> 
> They should just create a non-write-combining mapping if they
> don't support it.
> 

I wouldn't expect that since the caller function could be mislead to
believe it has a write combining capable area.

Re: [PATCH 13/17] mlx4: Add blue flame support for kernel consumers

[David Miller]

From: Eli Cohen <eli@dev.mellanox.co.il>
Date: Mon, 7 Mar 2011 23:48:12 +0200

> On Mon, Mar 07, 2011 at 01:40:01PM -0800, David Miller wrote:
>> From: Eli Cohen <eli@dev.mellanox.co.il>
>> Date: Mon, 7 Mar 2011 23:36:48 +0200
>> 
>> > Specifically, some archs may not support write combining.
>> 
>> They should just create a non-write-combining mapping if they
>> don't support it.
>> 
> 
> I wouldn't expect that since the caller function could be mislead to
> believe it has a write combining capable area.

It's a performance optimization, if you don't get write combining you'll
get more strict ordering, rather than less.

It cannot cause problem.

Re: [PATCH 13/17] mlx4: Add blue flame support for kernel consumers

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Mon, 07 Mar 2011 13:49:31 -0800 (PST)

> From: Eli Cohen <eli@dev.mellanox.co.il>
> Date: Mon, 7 Mar 2011 23:48:12 +0200
> 
>> On Mon, Mar 07, 2011 at 01:40:01PM -0800, David Miller wrote:
>>> From: Eli Cohen <eli@dev.mellanox.co.il>
>>> Date: Mon, 7 Mar 2011 23:36:48 +0200
>>> 
>>> > Specifically, some archs may not support write combining.
>>> 
>>> They should just create a non-write-combining mapping if they
>>> don't support it.
>>> 
>> 
>> I wouldn't expect that since the caller function could be mislead to
>> believe it has a write combining capable area.
> 
> It's a performance optimization, if you don't get write combining you'll
> get more strict ordering, rather than less.
> 
> It cannot cause problem.

BTW, if we did as you suggest, fail if we don't support write combining,
then half the drivers in the tree would fail to probe on sparc64.

Every other driver expects it to succeed, with either write-combining
or more strict ordering semantics.  Never to fail simply because
write-combining isn't supported.

It's a request, not a requirement.

Re: [PATCH 13/17] mlx4: Add blue flame support for kernel consumers

[Eli Cohen]

On Mon, Mar 07, 2011 at 01:49:31PM -0800, David Miller wrote:
> 
> It's a performance optimization, if you don't get write combining you'll
> get more strict ordering, rather than less.
> 
> It cannot cause problem.

I agree, but the function could still fail and the caller's logic
could attempt to call ioreamp or take other action. For example, in
the case of blue flame, it is better performance-wise to avoid using
this feature if write combining is not available.

Re: [PATCH 13/17] mlx4: Add blue flame support for kernel consumers

[David Miller]

From: Eli Cohen <eli@dev.mellanox.co.il>
Date: Mon, 7 Mar 2011 23:58:03 +0200

> On Mon, Mar 07, 2011 at 01:49:31PM -0800, David Miller wrote:
>> 
>> It's a performance optimization, if you don't get write combining you'll
>> get more strict ordering, rather than less.
>> 
>> It cannot cause problem.
> 
> I agree, but the function could still fail and the caller's logic
> could attempt to call ioreamp or take other action. For example, in
> the case of blue flame, it is better performance-wise to avoid using
> this feature if write combining is not available.

It could, but the less complicated the interfaces the better.