Re: [PATCH for-next 0/8] Add Mellanox mlx5 driver for Connect-IB devices

Re: [PATCH for-next 0/8] Add Mellanox mlx5 driver for Connect-IB devices

[Roland Dreier]

So I'm inclined to apply the mlx5 driver for 3.11, since it's a
completely new driver.  However, reading through it so far I had the
following comments, and I'd like these cleanups addressed along with
Dave Miller's:

- The debug mask complexity seems unnecessary now that pr_debug() is
controllable at runtime with the DYNAMIC_DEBUG stuff.  We should get
rid of the extra level of indirection.

- I think the active flag for the health check timer is unnecessary.
It can just be stopped with del_timer_sync().

- Many places use "foo_spl" as a name, and in the Linux kernel
"foo_lock" would be much more idiomatic and easier to read.

- In:

+struct mlx5_cmd {
...
+    struct mlx5_cmd_stats    stats[0x80a];

the 0x80a magic number really needs to have a name.

 - R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-next 0/8] Add Mellanox mlx5 driver for Connect-IB devices

[Roland Dreier]

In general I don't think overriding the CFLAGS (as you do in the mlx5
Makefiles) is a good idea, and in particular here your -Wall -Werror
break the build, at least for my gcc 4.7.3:

  CC      drivers/infiniband/hw/mlx5/qp.o
/home/roland/Src/linux-merge.git/drivers/infiniband/hw/mlx5/qp.c: In
function ‘sq_overhead’:
/home/roland/Src/linux-merge.git/drivers/infiniband/hw/mlx5/qp.c:234:2:
error: case value ‘4671’ not in enumerated type ‘enum ib_qp_type’
[-Werror=switch]
/home/roland/Src/linux-merge.git/drivers/infiniband/hw/mlx5/qp.c: In
function ‘qp_has_rq’:
/home/roland/Src/linux-merge.git/drivers/infiniband/hw/mlx5/qp.c:334:20:
error: comparison between ‘enum ib_qp_type’ and ‘enum <anonymous>’
[-Werror=enum-compare]
/home/roland/Src/linux-merge.git/drivers/infiniband/hw/mlx5/qp.c: In
function ‘to_mlx5_st’:
/home/roland/Src/linux-merge.git/drivers/infiniband/hw/mlx5/qp.c:464:2:
error: case value ‘4671’ not in enumerated type ‘enum ib_qp_type’
[-Werror=switch]
/home/roland/Src/linux-merge.git/drivers/infiniband/hw/mlx5/qp.c: In
function ‘create_kernel_qp’:
/home/roland/Src/linux-merge.git/drivers/infiniband/hw/mlx5/qp.c:613:25:
error: comparison between ‘enum ib_qp_type’ and ‘enum <anonymous>’
[-Werror=enum-compare]
/home/roland/Src/linux-merge.git/drivers/infiniband/hw/mlx5/qp.c: In
function ‘create_qp_common’:
/home/roland/Src/linux-merge.git/drivers/infiniband/hw/mlx5/qp.c:815:25:
error: comparison between ‘enum ib_qp_type’ and ‘enum <anonymous>’
[-Werror=enum-compare]
/home/roland/Src/linux-merge.git/drivers/infiniband/hw/mlx5/qp.c: In
function ‘get_cqs’:
/home/roland/Src/linux-merge.git/drivers/infiniband/hw/mlx5/qp.c:975:2:
error: case value ‘4671’ not in enumerated type ‘enum ib_qp_type’
[-Werror=switch]
/home/roland/Src/linux-merge.git/drivers/infiniband/hw/mlx5/qp.c: In
function ‘ib_qp_type_str’:
/home/roland/Src/linux-merge.git/drivers/infiniband/hw/mlx5/qp.c:1064:2:
error: case value ‘4671’ not in enumerated type ‘enum ib_qp_type’
[-Werror=switch]
/home/roland/Src/linux-merge.git/drivers/infiniband/hw/mlx5/qp.c: In
function ‘mlx5_ib_create_qp’:
/home/roland/Src/linux-merge.git/drivers/infiniband/hw/mlx5/qp.c:1086:26:
error: comparison between ‘enum ib_qp_type’ and ‘enum <anonymous>’
[-Werror=enum-compare]
/home/roland/Src/linux-merge.git/drivers/infiniband/hw/mlx5/qp.c:1113:2:
error: case value ‘4671’ not in enumerated type ‘enum ib_qp_type’
[-Werror=switch]
/home/roland/Src/linux-merge.git/drivers/infiniband/hw/mlx5/qp.c: In
function ‘__mlx5_ib_modify_qp’:
/home/roland/Src/linux-merge.git/drivers/infiniband/hw/mlx5/qp.c:1452:20:
error: comparison between ‘enum ib_qp_type’ and ‘enum <anonymous>’
[-Werror=enum-compare]
/home/roland/Src/linux-merge.git/drivers/infiniband/hw/mlx5/qp.c: In
function ‘mlx5_ib_modify_qp’:
/home/roland/Src/linux-merge.git/drivers/infiniband/hw/mlx5/qp.c:1615:20:
error: comparison between ‘enum ib_qp_type’ and ‘enum <anonymous>’
[-Werror=enum-compare]
/home/roland/Src/linux-merge.git/drivers/infiniband/hw/mlx5/qp.c: In
function ‘mlx5_ib_post_send’:
/home/roland/Src/linux-merge.git/drivers/infiniband/hw/mlx5/qp.c:2166:19:
error: comparison between ‘enum ib_wr_opcode’ and ‘enum <anonymous>’
[-Werror=enum-compare]
/home/roland/Src/linux-merge.git/drivers/infiniband/hw/mlx5/qp.c:2165:3:
error: case value ‘4671’ not in enumerated type ‘enum ib_qp_type’
[-Werror=switch]


What is this IB_QPT_REG_UMR stuff anyway?  Shouldn't we strip out all
that from the mlx5 driver until it's available in the core code?
Especially stuff like

/*
 * we do not expose this yet so we use a value out of range */
enum {
        IB_QPT_REG_UMR = IB_QPT_MAX + 0x1234,
};

/* ===> this should be passed to the vergbs layer */
enum {
        IB_WR_SET_PSV = IB_WR_BIND_MW + 10,
        IB_WR_GET_PSV,
        IB_WR_CHECK_PSV,
        IB_WR_RGET_PSV,
        IB_WR_RCHECK_PSV,
        IB_WR_UMR,
};

enum {
        IB_SEND_UMR_UNREG       = IB_SEND_IP_CSUM << 1,
};

enum ib_latency_class {
        IB_LATENCY_CLASS_LOW,
        IB_LATENCY_CLASS_MEDIUM,
        IB_LATENCY_CLASS_HIGH,
        IB_LATENCY_CLASS_FAST_PATH
};
/* <=== this should be passed to the vergbs layer */

looks like it shouldn't be in your submission.  (What are "vergbs" anyway? :)
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-next 0/8] Add Mellanox mlx5 driver for Connect-IB devices

[Joe Perches]

On Mon, 2013-07-01 at 10:49 -0700, Roland Dreier wrote:
> So I'm inclined to apply the mlx5 driver for 3.11, since it's a
> completely new driver.  However, reading through it so far I had the
> following comments, and I'd like these cleanups addressed along with
> Dave Miller's:
> 
> - The debug mask complexity seems unnecessary now that pr_debug() is
> controllable at runtime with the DYNAMIC_DEBUG stuff.  We should get
> rid of the extra level of indirection.

There's some value in block enabling/disabling messages
that dynamic_debug doesn't currently offer.

> - In:
> 
> +struct mlx5_cmd {
> ...
> +    struct mlx5_cmd_stats    stats[0x80a];
> 
> the 0x80a magic number really needs to have a name.

And that's a pretty big struct too.
2058 array entries. ~100kb
Does it really need to be that big?

Re: [PATCH for-next 0/8] Add Mellanox mlx5 driver for Connect-IB devices

[Roland Dreier]

On Mon, Jul 1, 2013 at 11:03 AM, Joe Perches <joe@perches.com> wrote:
> There's some value in block enabling/disabling messages
> that dynamic_debug doesn't currently offer.

As far as I can see, the mlx5 stuff ends up being per-file.  Which
dynamic debug already does offer.

 - R.

Re: [PATCH for-next 0/8] Add Mellanox mlx5 driver for Connect-IB devices

[Joe Perches]

On Mon, 2013-07-01 at 11:11 -0700, Roland Dreier wrote:
> On Mon, Jul 1, 2013 at 11:03 AM, Joe Perches <joe@perches.com> wrote:
> > There's some value in block enabling/disabling messages
> > that dynamic_debug doesn't currently offer.
> 
> As far as I can see, the mlx5 stuff ends up being per-file.  Which
> dynamic debug already does offer.

I didn't look too closely.

I do think that all __func__, __LINE__ and pid bits are
useless as it's all duplicated dynamic_debug functionality.

+#define mlx5_core_dbg(dev, format, arg...)                                    \
+do {                                                                          \
+       if (debug_mask & mlx5_core_debug_mask)                                 \
+               pr_debug("%s:%s:%d:(pid %d): " format, (dev)->priv.name,       \
+                        __func__, __LINE__, current->pid, ##arg);             \
+} while (0)
+
+#define mlx5_core_dbg_mask(dev, mask, format, arg...)                         \
+do {                                                                          \
+       if ((mask) & mlx5_core_debug_mask)                                     \
+               pr_debug("%s:%s:%d:(pid %d): " format, (dev)->priv.name,       \
+                        __func__, __LINE__, current->pid, ##arg);             \
+} while (0)

btw:

mlx5_core_dbg should just be

#define mlx5_core_dbg(dev, fmt, ...)				\
	mlx5_core_dbg_mask(dev, debug_mask, fmt, ##__VA_ARGS__)

[]

I think these are the groupings.

+enum {
+       MLX5_MOD_MAIN,
+       MLX5_MOD_CMDIF,
+       MLX5_MOD_EQ,
+       MLX5_MOD_QP,
+       MLX5_MOD_PGALLOC,
+       MLX5_MOD_FW,
+       MLX5_MOD_UAR,
+       MLX5_MOD_ALLOC,
+       MLX5_MOD_DEBUG,
+       MLX5_MOD_HEALTH,
+       MLX5_MOD_MAD,
+       MLX5_MOD_MCG,
+       MLX5_MOD_MR,
+       MLX5_MOD_PD,
+       MLX5_MOD_PORT,
+       MLX5_MOD_SRQ,
+       MLX5_MOD_CQ,
+       MLX5_MOD_CMD_DATA, /* print command payload only */
+       MLX5_CMD_DATA_TIME,
+};
+

Re: [PATCH for-next 0/8] Add Mellanox mlx5 driver for Connect-IB devices

[Roland Dreier]

On Mon, Jul 1, 2013 at 1:19 PM, Joe Perches <joe@perches.com> wrote:
> I think these are the groupings.
>
> +enum {
> +       MLX5_MOD_MAIN,
> +       MLX5_MOD_CMDIF,
> +       MLX5_MOD_EQ,
> +       MLX5_MOD_QP,
> +       MLX5_MOD_PGALLOC,
> +       MLX5_MOD_FW,
> +       MLX5_MOD_UAR,
> +       MLX5_MOD_ALLOC,
> +       MLX5_MOD_DEBUG,
> +       MLX5_MOD_HEALTH,
> +       MLX5_MOD_MAD,
> +       MLX5_MOD_MCG,
> +       MLX5_MOD_MR,
> +       MLX5_MOD_PD,
> +       MLX5_MOD_PORT,
> +       MLX5_MOD_SRQ,
> +       MLX5_MOD_CQ,
> +       MLX5_MOD_CMD_DATA, /* print command payload only */
> +       MLX5_CMD_DATA_TIME,
> +};


Right, but then look how they're used.  For example,
drivers/net/ethernet/mellanox/mlx5/core/main.c has:

    MLX5_MOD_DBG_MASK(MLX5_MOD_MAIN);

so MLX5_MOD_MAIN just means messages in main.c, etc.

 - R.

Re: [PATCH for-next 0/8] Add Mellanox mlx5 driver for Connect-IB devices

[Roland Dreier]

Also, sparse warns about the following.  It seems there's an endianness bug in

int mlx5_ib_umem_populate_pas(struct mlx5_ib_dev *dev, struct ib_umem *umem,
                  int npages, u64 *pas)

in mem.c, since it doesn't match what

void mlx5_ib_populate_pas(struct mlx5_ib_dev *dev, struct ib_umem *umem,
              int page_shift, __be64 *pas, int umr)

does, nor does it match the declaration

int mlx5_ib_umem_populate_pas(struct mlx5_ib_dev *dev, struct ib_umem *umem,
                  int npages, __be64 *pas);

in mlx5_ib.h.  Nor does it have any callers, so it's a bit hard to
tell if it's really and truly a bug.

 - R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-next 0/8] Add Mellanox mlx5 driver for Connect-IB devices

[Joe Perches]

On Mon, 2013-07-01 at 14:20 -0700, Roland Dreier wrote:
> On Mon, Jul 1, 2013 at 1:19 PM, Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> wrote:
> > I think these are the groupings.
> >
> > +enum {
> > +       MLX5_MOD_MAIN,
> > +       MLX5_MOD_CMDIF,
> > +       MLX5_MOD_EQ,
> > +       MLX5_MOD_QP,
> > +       MLX5_MOD_PGALLOC,
> > +       MLX5_MOD_FW,
> > +       MLX5_MOD_UAR,
> > +       MLX5_MOD_ALLOC,
> > +       MLX5_MOD_DEBUG,
> > +       MLX5_MOD_HEALTH,
> > +       MLX5_MOD_MAD,
> > +       MLX5_MOD_MCG,
> > +       MLX5_MOD_MR,
> > +       MLX5_MOD_PD,
> > +       MLX5_MOD_PORT,
> > +       MLX5_MOD_SRQ,
> > +       MLX5_MOD_CQ,
> > +       MLX5_MOD_CMD_DATA, /* print command payload only */
> > +       MLX5_CMD_DATA_TIME,
> > +};
> 
> 
> Right, but then look how they're used.  For example,
> drivers/net/ethernet/mellanox/mlx5/core/main.c has:
> 
>     MLX5_MOD_DBG_MASK(MLX5_MOD_MAIN);
> 
> so MLX5_MOD_MAIN just means messages in main.c, etc.

Thanks.  OK, yeah, that's completely unnecessary.
Just using dynamic_debug is far better.


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-next 0/8] Add Mellanox mlx5 driver for Connect-IB devices

[Or Gerlitz]

On Tue, Jul 2, 2013 at 12:22 AM, Roland Dreier <roland-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> Also, sparse warns about [...] in mlx5_ib.h.  Nor does it have any callers, so it's a bit
> hard to tell if it's really and truly a bug.

removing this function for V2
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-next 0/8] Add Mellanox mlx5 driver for Connect-IB devices

[Or Gerlitz]

On Mon, Jul 1, 2013 at 9:03 PM, Joe Perches <joe@perches.com> wrote:
> On Mon, 2013-07-01 at 10:49 -0700, Roland Dreier wrote:

[...]
>> +struct mlx5_cmd {
>> ...
>> +    struct mlx5_cmd_stats    stats[0x80a];
>> the 0x80a magic number really needs to have a name.

> And that's a pretty big struct too. 2058 array entries. ~100kb
> Does it really need to be that big?

Indeed a bit of biggish, but note there's just one instance of this
arrary per HCA port. For now we will leave this size but make it less
obscure, will be MLX5_CMD_OP_LAST

Re: [PATCH for-next 0/8] Add Mellanox mlx5 driver for Connect-IB devices

[Or Gerlitz]

On Mon, Jul 1, 2013 at 9:03 PM, Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> wrote:
> On Mon, 2013-07-01 at 10:49 -0700, Roland Dreier wrote:

On Mon, Jul 1, 2013 at 9:03 PM, Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> wrote:
> On Mon, 2013-07-01 at 10:49 -0700, Roland Dreier wrote:

[...]
>> +struct mlx5_cmd {
>> ...
>> +    struct mlx5_cmd_stats    stats[0x80a];
>> the 0x80a magic number really needs to have a name.

> And that's a pretty big struct too. 2058 array entries. ~100kb
> Does it really need to be that big?

Indeed a bit of biggish, but note there's just one instance of this
arrary per HCA port. For now we will leave this size but make it less
obscure, will be MLX5_CMD_OP_LAST
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-next 0/8] Add Mellanox mlx5 driver for Connect-IB devices

[Or Gerlitz]

On Mon, Jul 1, 2013 at 9:03 PM, Roland Dreier <roland-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> In general I don't think overriding the CFLAGS (as you do in the mlx5
> Makefiles) is a good idea, and in particular here your -Wall -Werror
> break the build, at least for my gcc 4.7.3:
>
>   CC      drivers/infiniband/hw/mlx5/qp.o
> /home/roland/Src/linux-merge.git/drivers/infiniband/hw/mlx5/qp.c: In
> function ‘sq_overhead’:
> /home/roland/Src/linux-merge.git/drivers/infiniband/hw/mlx5/qp.c:234:2:
> error: case value ‘4671’ not in enumerated type ‘enum ib_qp_type’
> [-Werror=switch]


Will do both (A) remove the flags added on the driver makefile and (B)
fix the issues pointed by these flags...

[...]

> What is this IB_QPT_REG_UMR stuff anyway?  Shouldn't we strip out all
> that from the mlx5 driver until it's available in the core code?

IB_QPT_REG_UMR is the type of QP used internally by the driver, to do
plain memory registration by verbs consumers. Will apply here a
similar practice to the one done by mlx4 driver to create the proxy
and tunnel QP types  for SRIOV, e.g will define MLX5_IB_QPT_REG_UMR
and use that under driver specific QP creation flags for which we have
the foundations in the IB verbs header file to go and use.

[...]

> /* ===> this should be passed to the vergbs layer */
> enum {
>         IB_WR_SET_PSV = IB_WR_BIND_MW + 10,
>         IB_WR_GET_PSV,
>         IB_WR_CHECK_PSV,
>         IB_WR_RGET_PSV,
>         IB_WR_RCHECK_PSV,
>         IB_WR_UMR,
> };
>
> enum {
>         IB_SEND_UMR_UNREG       = IB_SEND_IP_CSUM << 1,
> };
>
> enum ib_latency_class {
>         IB_LATENCY_CLASS_LOW,
>         IB_LATENCY_CLASS_MEDIUM,
>         IB_LATENCY_CLASS_HIGH,
>         IB_LATENCY_CLASS_FAST_PATH
> };
> /* <=== this should be passed to the vergbs layer */
>
> looks like it shouldn't be in your submission.  (What are "vergbs" anyway? :)

Will fix that, basically, will remove things we can get along for now,
e.g unused, even not internally such as IB_WR_YYY_PSV, and internalize
what we do need internally e.g use MLX5_IB_XXX where IB_XXX was used

and "vergbs" is a typo whose fix missed the version submitted...
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-next 0/8] Add Mellanox mlx5 driver for Connect-IB devices

[Or Gerlitz]

On Mon, Jul 1, 2013 at 8:49 PM, Roland Dreier <roland@kernel.org> wrote:
> So I'm inclined to apply the mlx5 driver for 3.11, since it's a
> completely new driver.  However, reading through it so far I had the
> following comments, and I'd like these cleanups addressed along with Dave Miller's:

Roland,

Working to have all Dave Miller's comments addressed along with yours
and post V2 later this week, so we will be still on track for a 3.11
merge of the core and IB driver through your tree.


> - The debug mask complexity seems unnecessary now that pr_debug() is
> controllable at runtime with the DYNAMIC_DEBUG stuff.  We should get
> rid of the extra level of indirection.

OK

> - I think the active flag for the health check timer is unnecessary.
> It can just be stopped with del_timer_sync().

Jack was looking on this today and we're not sure, he will send his
reading of the matter tomorrow.

> - Many places use "foo_spl" as a name, and in the Linux kernel
> "foo_lock" would be much more idiomatic and easier to read.

sure, done.

>
> - In:
>
> +struct mlx5_cmd {
> ...
> +    struct mlx5_cmd_stats    stats[0x80a];
>
> the 0x80a magic number really needs to have a name.

done.

Re: [PATCH for-next 0/8] Add Mellanox mlx5 driver for Connect-IB devices

[Or Gerlitz]

On 01/07/2013 20:49, Roland Dreier wrote:
> - I think the active flag for the health check timer is unnecessary.
> It can just be stopped with del_timer_sync().

Hi Roland

Jack looked on this comment/code and he says that the active flag is used
to prevent re-scheduling the timer from inside the timer handling routine.

In the kernel, the comment header in the source file for del_timer_sync
explicitly states that re-scheduling the timer must be prevented,
or the sync is useless:Callers must prevent restarting of the timer, 
otherwise
this function is meaningless

So we believe that code should remain.

Or.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-next 0/8] Add Mellanox mlx5 driver for Connect-IB devices

[Roland Dreier]

On Wed, Jul 3, 2013 at 9:41 AM, Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> wrote:
> Jack looked on this comment/code and he says that the active flag is used
> to prevent re-scheduling the timer from inside the timer handling routine.
>
> In the kernel, the comment header in the source file for del_timer_sync
> explicitly states that re-scheduling the timer must be prevented,
> or the sync is useless:Callers must prevent restarting of the timer,
> otherwise
> this function is meaningless
>
> So we believe that code should remain.

Look at the actual timer code.  del_timer_sync() won't work if
something unrelated re-adds the timer, but it will work if the timer
itself is what re-adds itself.

Documentation/DocBook/kernel-locking.tmpl says:

      Another common problem is deleting timers which restart
      themselves (by calling <function>add_timer()</function> at the end
      of their timer function).  Because this is a fairly common case
      which is prone to races, you should use
<function>del_timer_sync()</function>
      (<filename class="headerfile">include/linux/timer.h</filename>)
      to handle this case.  It returns the number of times the timer
      had to be deleted before we finally stopped it from adding itself back
      in.

which pretty clearly says that del_timer_sync() will work in this case.

Or look at the code using it in arch/sparc/kernel/led.c for example
(just one of the first hits in my grep, there are many other
examples).

Not a big deal but I'm pretty sure the flag isn't needed.

 - R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-next 0/8] Add Mellanox mlx5 driver for Connect-IB devices

[Or Gerlitz]

On Wed, Jul 3, 2013 at 10:26 PM, Roland Dreier <roland@kernel.org> wrote:
> On Wed, Jul 3, 2013 at 9:41 AM, Or Gerlitz <ogerlitz@mellanox.com> wrote:
> > Jack looked on this comment/code and he says that the active flag is used
> > to prevent re-scheduling the timer from inside the timer handling routine.
> >
> > In the kernel, the comment header in the source file for del_timer_sync
> > explicitly states that re-scheduling the timer must be prevented,
> > or the sync is useless:Callers must prevent restarting of the timer,
> > otherwise
> > this function is meaningless
> >
> > So we believe that code should remain.
>
> Look at the actual timer code.  del_timer_sync() won't work if
> something unrelated re-adds the timer, but it will work if the timer
> itself is what re-adds itself.

[...]

OK, we will re-look into that tomorrow. So how V2 looks?

Or.

Re: [PATCH for-next 0/8] Add Mellanox mlx5 driver for Connect-IB devices

[Jack Morgenstein]

On Wednesday 03 July 2013 22:26, Roland Dreier wrote:
> Look at the actual timer code.  del_timer_sync() won't work if
> something unrelated re-adds the timer, but it will work if the timer
> itself is what re-adds itself.
> 
> Documentation/DocBook/kernel-locking.tmpl says:
> 
>       Another common problem is deleting timers which restart
>       themselves (by calling <function>add_timer()</function> at the end
>       of their timer function).  Because this is a fairly common case
>       which is prone to races, you should use
> <function>del_timer_sync()</function>
>       (<filename class="headerfile">include/linux/timer.h</filename>)
>       to handle this case.  It returns the number of times the timer
>       had to be deleted before we finally stopped it from adding itself back
>       in.
> 
> which pretty clearly says that del_timer_sync() will work in this case.
> 
> Or look at the code using it in arch/sparc/kernel/led.c for example
> (just one of the first hits in my grep, there are many other
> examples).
> 
> Not a big deal but I'm pretty sure the flag isn't needed.

Thanks for the feedback, Roland!

You are correct, and I removed the "active" flag in V3 of the patch set (to be submitted shortly).

-Jack
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html