diff mbox series

[RFC,1/6] eal: introduce macros for getting value for bit

Message ID 20200610171728.89-2-parav@mellanox.com (mailing list archive)
State Superseded, archived
Headers
Series Improve mlx5 PMD common driver framework for multiple classes |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Parav Pandit June 10, 2020, 5:17 p.m. UTC 
  There are several drivers which duplicate bit generation macro.
Introduce a generic bit macros so that such drivers avoid redefining
same in multiple drivers.

Signed-off-by: Parav Pandit <parav@mellanox.com>
---
 lib/librte_eal/include/rte_bits.h | 10 ++++++++++
 1 file changed, 10 insertions(+)
 create mode 100644 lib/librte_eal/include/rte_bits.h

Comments

Gaëtan Rivet June 15, 2020, 7:33 p.m. UTC | #1 
Hello Parav,

On 10/06/20 17:17 +0000, Parav Pandit wrote:
> There are several drivers which duplicate bit generation macro.
> Introduce a generic bit macros so that such drivers avoid redefining
> same in multiple drivers.
> 
> Signed-off-by: Parav Pandit <parav@mellanox.com>
> ---
>  lib/librte_eal/include/rte_bits.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>  create mode 100644 lib/librte_eal/include/rte_bits.h
> 
> diff --git a/lib/librte_eal/include/rte_bits.h b/lib/librte_eal/include/rte_bits.h
> new file mode 100644
> index 000000000..37f284971
> --- /dev/null
> +++ b/lib/librte_eal/include/rte_bits.h
> @@ -0,0 +1,10 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright 2020 Mellanox Technologies, Ltd.
> + */
> +
> +#ifndef _RTE_BITS_H_
> +#define _RTE_BITS_H_
> +
> +#define RTE_BIT(bit_num)	(1UL << (bit_num))
                           ^ The tab here should be replaced by a space.
> +
> +#endif
> -- 
> 2.25.4
> 

I'm not sure this kind of macro is needed, but if multiple
drivers are using the patterns let's say ok.

However I don't think it needs its own header. Would it be ok in
lib/librte_eal/include/rte_common.h for example?
Thomas Monjalon June 17, 2020, 8:05 a.m. UTC | #2 
15/06/2020 21:33, Gaëtan Rivet:
> On 10/06/20 17:17 +0000, Parav Pandit wrote:
> > There are several drivers which duplicate bit generation macro.
> > Introduce a generic bit macros so that such drivers avoid redefining
> > same in multiple drivers.
> > 
> > Signed-off-by: Parav Pandit <parav@mellanox.com>
> > ---
[...]
> > --- /dev/null
> > +++ b/lib/librte_eal/include/rte_bits.h
> > @@ -0,0 +1,10 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright 2020 Mellanox Technologies, Ltd.
> > + */
> > +
> > +#ifndef _RTE_BITS_H_
> > +#define _RTE_BITS_H_
> > +
> > +#define RTE_BIT(bit_num)	(1UL << (bit_num))
>                            ^ The tab here should be replaced by a space.
> > +
> > +#endif
> 
> I'm not sure this kind of macro is needed, but if multiple
> drivers are using the patterns let's say ok.
> 
> However I don't think it needs its own header. Would it be ok in
> lib/librte_eal/include/rte_common.h for example?

If we want to reuse an existing file, it could be
lib/librte_eal/include/rte_bitops.h
Parav Pandit June 18, 2020, 9:25 a.m. UTC | #3 
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Wednesday, June 17, 2020 1:35 PM
> 
> 15/06/2020 21:33, Gaëtan Rivet:
> > On 10/06/20 17:17 +0000, Parav Pandit wrote:
> > > There are several drivers which duplicate bit generation macro.
> > > Introduce a generic bit macros so that such drivers avoid redefining
> > > same in multiple drivers.
> > >
> > > Signed-off-by: Parav Pandit <parav@mellanox.com>
> > > ---
> [...]
> > > --- /dev/null
> > > +++ b/lib/librte_eal/include/rte_bits.h
> > > @@ -0,0 +1,10 @@
> > > +/* SPDX-License-Identifier: BSD-3-Clause
> > > + * Copyright 2020 Mellanox Technologies, Ltd.
> > > + */
> > > +
> > > +#ifndef _RTE_BITS_H_
> > > +#define _RTE_BITS_H_
> > > +
> > > +#define RTE_BIT(bit_num)	(1UL << (bit_num))
> >                            ^ The tab here should be replaced by a space.
> > > +
> > > +#endif
> >
> > I'm not sure this kind of macro is needed, but if multiple drivers are
> > using the patterns let's say ok.
> >
Yes. we certainly need it. Currently BIT() macro is used at 3000+ locations and defined in 10+ drivers.
Once we have this macro, it can gradually be replaced.

> > However I don't think it needs its own header. Would it be ok in
> > lib/librte_eal/include/rte_common.h for example?
> 
> If we want to reuse an existing file, it could be
> lib/librte_eal/include/rte_bitops.h
> 
o.k.
I will rename file from rte_bits.h to rte_bitops.h
More such macros will be available here to avoid redefinitions in drivers.
Parav Pandit June 18, 2020, 12:16 p.m. UTC | #4 
Hi Thomas,

> From: dev <dev-bounces@dpdk.org> On Behalf Of Parav Pandit
> Sent: Thursday, June 18, 2020 2:55 PM
> 
> > From: Thomas Monjalon <thomas@monjalon.net>
> > Sent: Wednesday, June 17, 2020 1:35 PM
> >
> > 15/06/2020 21:33, Gaëtan Rivet:
> > > On 10/06/20 17:17 +0000, Parav Pandit wrote:
> > > > There are several drivers which duplicate bit generation macro.
> > > > Introduce a generic bit macros so that such drivers avoid
> > > > redefining same in multiple drivers.
> > > >
> > > > Signed-off-by: Parav Pandit <parav@mellanox.com>
> > > > ---
> > [...]
> > > > --- /dev/null
> > > > +++ b/lib/librte_eal/include/rte_bits.h
> > > > @@ -0,0 +1,10 @@
> > > > +/* SPDX-License-Identifier: BSD-3-Clause
> > > > + * Copyright 2020 Mellanox Technologies, Ltd.
> > > > + */
> > > > +
> > > > +#ifndef _RTE_BITS_H_
> > > > +#define _RTE_BITS_H_
> > > > +
> > > > +#define RTE_BIT(bit_num)	(1UL << (bit_num))
> > >                            ^ The tab here should be replaced by a space.
> > > > +
> > > > +#endif
> > >
> > > I'm not sure this kind of macro is needed, but if multiple drivers
> > > are using the patterns let's say ok.
> > >
> Yes. we certainly need it. Currently BIT() macro is used at 3000+ locations and
> defined in 10+ drivers.
> Once we have this macro, it can gradually be replaced.
> 
> > > However I don't think it needs its own header. Would it be ok in
> > > lib/librte_eal/include/rte_common.h for example?
> >
> > If we want to reuse an existing file, it could be
> > lib/librte_eal/include/rte_bitops.h
> >
> o.k.
> I will rename file from rte_bits.h to rte_bitops.h More such macros will be
> available here to avoid redefinitions in drivers.

I see that rte_bitops.h already exist and this new definition doesn't fit well in the rte_bitops.h.
The intent of the rte_bitops.h is to have generic mostly macros to replace current duplication of:

Such as
BIT()
BIT_ULL()
BITS_PER_BYTE()
BITS_TO_LONGS()

They do not fit well in rte_bitops.h which works on the 'bitmap'.
Thomas Monjalon June 18, 2020, 12:22 p.m. UTC | #5 
18/06/2020 14:16, Parav Pandit:
> From: Parav Pandit
> > From: Thomas Monjalon <thomas@monjalon.net>
> > > 15/06/2020 21:33, Gaëtan Rivet:
> > > > On 10/06/20 17:17 +0000, Parav Pandit wrote:
> > > > > There are several drivers which duplicate bit generation macro.
> > > > > Introduce a generic bit macros so that such drivers avoid
> > > > > redefining same in multiple drivers.
> > > > >
> > > > > Signed-off-by: Parav Pandit <parav@mellanox.com>
> > > > > ---
> > > [...]
> > > > > --- /dev/null
> > > > > +++ b/lib/librte_eal/include/rte_bits.h
> > > > > @@ -0,0 +1,10 @@
> > > > > +/* SPDX-License-Identifier: BSD-3-Clause
> > > > > + * Copyright 2020 Mellanox Technologies, Ltd.
> > > > > + */
> > > > > +
> > > > > +#ifndef _RTE_BITS_H_
> > > > > +#define _RTE_BITS_H_
> > > > > +
> > > > > +#define RTE_BIT(bit_num)	(1UL << (bit_num))
> > > >                            ^ The tab here should be replaced by a space.
> > > > > +
> > > > > +#endif
> > > >
> > > > I'm not sure this kind of macro is needed, but if multiple drivers
> > > > are using the patterns let's say ok.
> > > >
> > Yes. we certainly need it. Currently BIT() macro is used at 3000+ locations and
> > defined in 10+ drivers.
> > Once we have this macro, it can gradually be replaced.
> > 
> > > > However I don't think it needs its own header. Would it be ok in
> > > > lib/librte_eal/include/rte_common.h for example?
> > >
> > > If we want to reuse an existing file, it could be
> > > lib/librte_eal/include/rte_bitops.h
> > >
> > o.k.
> > I will rename file from rte_bits.h to rte_bitops.h More such macros will be
> > available here to avoid redefinitions in drivers.
> 
> I see that rte_bitops.h already exist and this new definition doesn't fit well in the rte_bitops.h.
> The intent of the rte_bitops.h is to have generic mostly macros to replace current duplication of:
> 
> Such as
> BIT()
> BIT_ULL()
> BITS_PER_BYTE()
> BITS_TO_LONGS()
> 
> They do not fit well in rte_bitops.h which works on the 'bitmap'.

Current bitops functions are getting and setting a bit.
I don't undertand why you say "works on the bitmap".
In my opinion, bit definition fits in this include file.
Parav Pandit June 18, 2020, 1:20 p.m. UTC | #6 
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Thursday, June 18, 2020 5:52 PM
> 
> 18/06/2020 14:16, Parav Pandit:
> > From: Parav Pandit
> > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > 15/06/2020 21:33, Gaëtan Rivet:
> > > > > On 10/06/20 17:17 +0000, Parav Pandit wrote:
> > > > > > There are several drivers which duplicate bit generation macro.
> > > > > > Introduce a generic bit macros so that such drivers avoid
> > > > > > redefining same in multiple drivers.
> > > > > >
> > > > > > Signed-off-by: Parav Pandit <parav@mellanox.com>
> > > > > > ---
> > > > [...]
> > > > > > --- /dev/null
> > > > > > +++ b/lib/librte_eal/include/rte_bits.h
> > > > > > @@ -0,0 +1,10 @@
> > > > > > +/* SPDX-License-Identifier: BSD-3-Clause
> > > > > > + * Copyright 2020 Mellanox Technologies, Ltd.
> > > > > > + */
> > > > > > +
> > > > > > +#ifndef _RTE_BITS_H_
> > > > > > +#define _RTE_BITS_H_
> > > > > > +
> > > > > > +#define RTE_BIT(bit_num)	(1UL << (bit_num))
> > > > >                            ^ The tab here should be replaced by a space.
> > > > > > +
> > > > > > +#endif
> > > > >
> > > > > I'm not sure this kind of macro is needed, but if multiple
> > > > > drivers are using the patterns let's say ok.
> > > > >
> > > Yes. we certainly need it. Currently BIT() macro is used at 3000+
> > > locations and defined in 10+ drivers.
> > > Once we have this macro, it can gradually be replaced.
> > >
> > > > > However I don't think it needs its own header. Would it be ok in
> > > > > lib/librte_eal/include/rte_common.h for example?
> > > >
> > > > If we want to reuse an existing file, it could be
> > > > lib/librte_eal/include/rte_bitops.h
> > > >
> > > o.k.
> > > I will rename file from rte_bits.h to rte_bitops.h More such macros
> > > will be available here to avoid redefinitions in drivers.
> >
> > I see that rte_bitops.h already exist and this new definition doesn't fit well
> in the rte_bitops.h.
> > The intent of the rte_bitops.h is to have generic mostly macros to replace
> current duplication of:
> >
> > Such as
> > BIT()
> > BIT_ULL()
> > BITS_PER_BYTE()
> > BITS_TO_LONGS()
> >
> > They do not fit well in rte_bitops.h which works on the 'bitmap'.
> 
> Current bitops functions are getting and setting a bit.
> I don't undertand why you say "works on the bitmap".
It sets in 32-bit and 64-bit addr.
Eventually it will be possibly extended to more than 64-bit. It will be a bitmap as generic API.

> In my opinion, bit definition fits in this include file.
> 
Ok. I am fine. I don't have strong opinion to create a new file.
I will place it here.
diff mbox series

Patch

diff --git a/lib/librte_eal/include/rte_bits.h b/lib/librte_eal/include/rte_bits.h
new file mode 100644
index 000000000..37f284971
--- /dev/null
+++ b/lib/librte_eal/include/rte_bits.h
@@ -0,0 +1,10 @@ 
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright 2020 Mellanox Technologies, Ltd.
+ */
+
+#ifndef _RTE_BITS_H_
+#define _RTE_BITS_H_
+
+#define RTE_BIT(bit_num)	(1UL << (bit_num))
+
+#endif