qdisc hash table changes...

qdisc hash table changes...

[David Miller]

I think there will still be build failures even with v6 due to symbol
clashes.

For example, kernel/audit_tree.c defines HASH_SIZE as an enumeration
value, and that (indirectly) includes networking headers.

There are others all over the tree.

I would therefore ask that you first fix the namespace conflicts
against the hash symbols in the entire tree as a separate patch series
(one for each driver/subsystem which has this problem.)  Really, get
it down to "git grep hash_add | grep -v _hash_add" and similar
returning no output.

Then we can add the qdisc hash facility.

Thanks.

Re: qdisc hash table changes...

[Jiri Kosina]

On Mon, 8 Aug 2016, David Miller wrote:

> I think there will still be build failures even with v6 due to symbol
> clashes.
> 
> For example, kernel/audit_tree.c defines HASH_SIZE as an enumeration
> value, and that (indirectly) includes networking headers.

It does indirectly pull in some networking headers, but it doesn't pull in 
netdevice.h (which is the place where the actual include of hashtable.h 
happens):

$ make kernel/audit_tree.i; grep HASH_SIZE kernel/audit_tree.i; grep netdev.*\.h kernel/audit_tree.i
  CHK     include/config/kernel.release
  CHK     include/generated/uapi/linux/version.h
  CHK     include/generated/utsrelease.h
  CHK     include/generated/bounds.h
  CHK     include/generated/timeconst.h
  CHK     include/generated/asm-offsets.h
  CALL    scripts/checksyscalls.sh
  DESCEND  objtool
  CPP     kernel/audit_tree.i
enum {HASH_SIZE = 128};
static struct list_head chunk_hash_heads[HASH_SIZE];
 return chunk_hash_heads + n % HASH_SIZE;
 for (i = 0; i < HASH_SIZE; i++)
# 1 "./include/linux/netdev_features.h" 1
# 15 "./include/linux/netdev_features.h"
# 85 "./include/linux/netdev_features.h"
struct sk_buff *__netdev_alloc_skb(struct net_device *dev, unsigned int length,
 return __netdev_alloc_skb(dev, length, ((( gfp_t)0x20u)|(( gfp_t)0x80000u)|(( gfp_t)0x2000000u)));
 return __netdev_alloc_skb(((void *)0), length, gfp_mask);
 return netdev_alloc_skb(((void *)0), length);
 struct sk_buff *skb = __netdev_alloc_skb(dev, length + 0, gfp);
 return __netdev_alloc_skb_ip_align(dev, length, ((( gfp_t)0x20u)|(( gfp_t)0x80000u)|(( gfp_t)0x2000000u)));

So this is fine.

> There are others all over the tree.
> 
> I would therefore ask that you first fix the namespace conflicts against 
> the hash symbols in the entire tree as a separate patch series (one for 
> each driver/subsystem which has this problem.)  Really, get it down to 
> "git grep hash_add | grep -v _hash_add" and similar returning no output.

Is this really worth the hassle when there are no real conflicts (i.e. the 
code in question doesn't indirectly pull in netdevice.h)?

I just did allmodconfig build with v6, and it passed. Fengguang's 0-day 
bot didn't report any failures either.

> Then we can add the qdisc hash facility.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: qdisc hash table changes...

[David Miller]

From: Jiri Kosina <jikos@kernel.org>
Date: Tue, 9 Aug 2016 09:42:01 +0200 (CEST)

> It does indirectly pull in some networking headers, but it doesn't pull in 
> netdevice.h (which is the place where the actual include of hashtable.h 
> happens):

It pulls in skbuff.h, so are you willing to bet forever that skbuff.h
won't indirectly pull in netdevice.h at some point in the future?

This is obviously a big problem, having global namespace conflicts
like this.

Pushing fixing it propering into the future isn't really the right
thing to do.

So please entertain my request to do this properly.

Thank you.

Re: qdisc hash table changes...

[Jiri Kosina]

On Tue, 9 Aug 2016, David Miller wrote:

> > It does indirectly pull in some networking headers, but it doesn't pull in 
> > netdevice.h (which is the place where the actual include of hashtable.h 
> > happens):
> 
> It pulls in skbuff.h, so are you willing to bet forever that skbuff.h
> won't indirectly pull in netdevice.h at some point in the future?
> 
> This is obviously a big problem, having global namespace conflicts
> like this.
> 
> Pushing fixing it propering into the future isn't really the right
> thing to do.
> 
> So please entertain my request to do this properly.

Ok, I will add this to my TODO list, but this can take quite some time 
(especially as I can imagine some maintainers pushing back on this, 
because it doesn't really fix any existing issue in their code).

Does that strictly have to be a show-stopper for the qdisc hash 
conversion, given the fact that the whole tree is building properly?

> Thank you.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: qdisc hash table changes...

[David Miller]

From: Jiri Kosina <jikos@kernel.org>
Date: Tue, 9 Aug 2016 11:02:43 +0200 (CEST)

> Does that strictly have to be a show-stopper for the qdisc hash 
> conversion, given the fact that the whole tree is building properly?

I guess not.  Please submit that change as a series, first patches
that correct the build required hash symbol conflict fixes, then
the qdisc change itself.

THanks.