| Message ID | 1421009571-5279-4-git-send-email-richard@nod.at |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Sun, 2015-01-11 at 21:52 +0100, Richard Weinberger wrote: > arp_tables.c has a 16bit aligment ifname_compare(), factor > it out to use it for all tables. [] > diff --git a/include/linux/netfilter/x_tables.h b/include/linux/netfilter/x_tables.h [] > @@ -331,14 +331,15 @@ static inline void xt_write_recseq_end(unsigned int addend) > /* > * This helper is performance critical and must be inlined > */ > -static inline unsigned long ifname_compare_aligned(const char *_a, > - const char *_b, > - const char *_mask) > +static inline unsigned long ifname_compare(const char *_a, > + const char *_b, > + const char *_mask) Perhaps this would be better as bool ifname_compare -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 11.01.2015 um 21:59 schrieb Joe Perches: > On Sun, 2015-01-11 at 21:52 +0100, Richard Weinberger wrote: >> arp_tables.c has a 16bit aligment ifname_compare(), factor >> it out to use it for all tables. > [] >> diff --git a/include/linux/netfilter/x_tables.h b/include/linux/netfilter/x_tables.h > [] >> @@ -331,14 +331,15 @@ static inline void xt_write_recseq_end(unsigned int addend) >> /* >> * This helper is performance critical and must be inlined >> */ >> -static inline unsigned long ifname_compare_aligned(const char *_a, >> - const char *_b, >> - const char *_mask) >> +static inline unsigned long ifname_compare(const char *_a, >> + const char *_b, >> + const char *_mask) > > Perhaps this would be better as bool ifname_compare Let's discuss the whole concept first, then we can go to bikeshedding mode. Thanks, //richard -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 2015-01-11 at 22:02 +0100, Richard Weinberger wrote: > Am 11.01.2015 um 21:59 schrieb Joe Perches: > > On Sun, 2015-01-11 at 21:52 +0100, Richard Weinberger wrote: > >> arp_tables.c has a 16bit aligment ifname_compare(), factor > >> it out to use it for all tables. [] > > Perhaps this would be better as bool ifname_compare > Let's discuss the whole concept first The concept seems obvious enough -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 11.01.2015 um 22:14 schrieb Joe Perches: > On Sun, 2015-01-11 at 22:02 +0100, Richard Weinberger wrote: >> Am 11.01.2015 um 21:59 schrieb Joe Perches: >>> On Sun, 2015-01-11 at 21:52 +0100, Richard Weinberger wrote: >>>> arp_tables.c has a 16bit aligment ifname_compare(), factor >>>> it out to use it for all tables. > [] >>> Perhaps this would be better as bool ifname_compare >> Let's discuss the whole concept first > > The concept seems obvious enough Anyway, I agree with Linus wrt. bool. https://lkml.org/lkml/2013/8/31/138 Thanks, //richard -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 2015-01-11 at 22:30 +0100, Richard Weinberger wrote: > Am 11.01.2015 um 22:14 schrieb Joe Perches: > > On Sun, 2015-01-11 at 22:02 +0100, Richard Weinberger wrote: > >> Am 11.01.2015 um 21:59 schrieb Joe Perches: > >>> On Sun, 2015-01-11 at 21:52 +0100, Richard Weinberger wrote: > >>>> arp_tables.c has a 16bit aligment ifname_compare(), factor > >>>> it out to use it for all tables. > > [] > >>> Perhaps this would be better as bool ifname_compare > >> Let's discuss the whole concept first > > > > The concept seems obvious enough > > Anyway, I agree with Linus wrt. bool. > https://lkml.org/lkml/2013/8/31/138 I don't. He was right when he wrote: https://lkml.org/lkml/2014/3/10/760 Linus Torvalds <> I guess I haven't gotten over my hatred of people playing games with them because support wasn't universal enough. But maybe it's approaching being irrational these days. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 11.01.2015 um 22:39 schrieb Joe Perches: > On Sun, 2015-01-11 at 22:30 +0100, Richard Weinberger wrote: >> Am 11.01.2015 um 22:14 schrieb Joe Perches: >>> On Sun, 2015-01-11 at 22:02 +0100, Richard Weinberger wrote: >>>> Am 11.01.2015 um 21:59 schrieb Joe Perches: >>>>> On Sun, 2015-01-11 at 21:52 +0100, Richard Weinberger wrote: >>>>>> arp_tables.c has a 16bit aligment ifname_compare(), factor >>>>>> it out to use it for all tables. >>> [] >>>>> Perhaps this would be better as bool ifname_compare >>>> Let's discuss the whole concept first >>> >>> The concept seems obvious enough >> >> Anyway, I agree with Linus wrt. bool. >> https://lkml.org/lkml/2013/8/31/138 > > I don't. He was right when he wrote: Joe, I really don't care. This is the least significant patch of the series. I'll no longer waste my time with that. Thanks, //richard -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sunday 2015-01-11 22:30, Richard Weinberger wrote: >>>> Perhaps this would be better as bool ifname_compare > >Anyway, I agree with Linus wrt. bool. >https://lkml.org/lkml/2013/8/31/138 Had the function return "bool", it would have been obvious enough what to do with its return type. A return type of "int" might have hinted towards negative-is-error (in general) or strcmpish values (functions doing string compare work). Now that it returns "unsigned long", one is pressed to look at the function body (not bad per se, but it is a hump) for the return value's semantics. Linus says bool is dangerous to the unsuspecting user — but so is "volatile", microwave ovens, etc. If the kernel really cared for entry-level coders, it would be written in something like MISRA C. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Richard Weinberger <richard@nod.at> Date: Sun, 11 Jan 2015 22:42:37 +0100 > Joe, I really don't care. This is the least significant > patch of the series. > I'll no longer waste my time with that. If you're not willing to fix stylistic issues now, then nobody should bother wasting their time on the high level issues of your patch. Just fix these things now rather than being difficult, this is a part of patch review that everyone has to do, not just you. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 12.01.2015 um 03:50 schrieb David Miller: > From: Richard Weinberger <richard@nod.at> > Date: Sun, 11 Jan 2015 22:42:37 +0100 > >> Joe, I really don't care. This is the least significant >> patch of the series. >> I'll no longer waste my time with that. > > If you're not willing to fix stylistic issues now, then nobody should > bother wasting their time on the high level issues of your patch. > > Just fix these things now rather than being difficult, this is a part > of patch review that everyone has to do, not just you. I apologize, it was not my intention to be difficult. Please note that the stylistic issue is not a warning produced by checkpatch.pl. If you and netfilter folks now prefer bool for such string compare functions I'll happily address this in v2 of my series. Thanks, //richard -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2015-01-12 at 09:18 +0100, Richard Weinberger wrote: > Am 12.01.2015 um 03:50 schrieb David Miller: > > From: Richard Weinberger <richard@nod.at> > > Date: Sun, 11 Jan 2015 22:42:37 +0100 > > > >> Joe, I really don't care. This is the least significant > >> patch of the series. > >> I'll no longer waste my time with that. > > > > If you're not willing to fix stylistic issues now, then nobody should > > bother wasting their time on the high level issues of your patch. > > > > Just fix these things now rather than being difficult, this is a part > > of patch review that everyone has to do, not just you. > > I apologize, it was not my intention to be difficult. No worries. The unsigned long return is kind of odd with a compare_<foo> name as those are generally, as Jan mentioned, signed comparison style return values. I'd probably use a different function name too bool ifname_equal(const char *a, const char *b, const char *mask) { } to try to make the return value more obvious too. > If you and netfilter folks now prefer bool > for such string compare functions I'll happily address this in > v2 of my series. Thanks -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/netfilter/x_tables.h b/include/linux/netfilter/x_tables.h index 15bda23..26dddc1 100644 --- a/include/linux/netfilter/x_tables.h +++ b/include/linux/netfilter/x_tables.h @@ -331,14 +331,15 @@ static inline void xt_write_recseq_end(unsigned int addend) /* * This helper is performance critical and must be inlined */ -static inline unsigned long ifname_compare_aligned(const char *_a, - const char *_b, - const char *_mask) +static inline unsigned long ifname_compare(const char *_a, + const char *_b, + const char *_mask) { + unsigned long ret; +#ifdef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS const unsigned long *a = (const unsigned long *)_a; const unsigned long *b = (const unsigned long *)_b; const unsigned long *mask = (const unsigned long *)_mask; - unsigned long ret; ret = (a[0] ^ b[0]) & mask[0]; if (IFNAMSIZ > sizeof(unsigned long)) @@ -348,11 +349,21 @@ static inline unsigned long ifname_compare_aligned(const char *_a, if (IFNAMSIZ > 3 * sizeof(unsigned long)) ret |= (a[3] ^ b[3]) & mask[3]; BUILD_BUG_ON(IFNAMSIZ > 4 * sizeof(unsigned long)); +#else + const u16 *a = (const u16 *)_a; + const u16 *b = (const u16 *)_b; + const u16 *mask = (const u16 *)_mask; + int i; + + ret = 0; + for (i = 0; i < IFNAMSIZ/sizeof(u16); i++) + ret |= (a[i] ^ b[i]) & mask[i]; +#endif return ret; } /* - * A wrapper around ifname_compare_aligned() to match against dev->name and + * A wrapper around ifname_compare() to match against dev->name and * dev->ifalias. */ static inline unsigned long ifname_compare_all(const struct net_device *dev, @@ -364,9 +375,9 @@ static inline unsigned long ifname_compare_all(const struct net_device *dev, if (!dev) goto out; - res = ifname_compare_aligned(dev->name, name, mask); + res = ifname_compare(dev->name, name, mask); if (unlikely(dev->ifalias && res)) - res = ifname_compare_aligned(dev->ifalias, name, mask); + res = ifname_compare(dev->ifalias, name, mask); out: return res; diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c index 457d4ed..c978691 100644 --- a/net/ipv4/netfilter/arp_tables.c +++ b/net/ipv4/netfilter/arp_tables.c @@ -76,39 +76,6 @@ static inline int arp_devaddr_compare(const struct arpt_devaddr_info *ap, return ret != 0; } -/* - * Unfortunately, _b and _mask are not aligned to an int (or long int) - * Some arches dont care, unrolling the loop is a win on them. - * For other arches, we only have a 16bit alignement. - */ -static unsigned long ifname_compare(const struct net_device *dev, - const char *_b, const char *_mask) -{ -#ifdef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS - unsigned long ret = ifname_compare_all(dev, _b, _mask); -#else - unsigned long ret = 0; - const u16 *a = (const u16 *)dev->name; - const u16 *b = (const u16 *)_b; - const u16 *mask = (const u16 *)_mask; - int i; - - for (i = 0; i < IFNAMSIZ/sizeof(u16); i++) - ret |= (a[i] ^ b[i]) & mask[i]; - - if (likely(!(dev->ifalias && ret))) - goto out; - - ret = 0; - a = (const u16 *)dev->ifalias; - for (i = 0; i < IFNAMSIZ/sizeof(u16); i++) - ret |= (a[i] ^ b[i]) & mask[i]; - -out: -#endif - return ret; -} - /* Returns whether packet matches rule or not. */ static inline int arp_packet_match(const struct arphdr *arphdr, struct net_device *dev, @@ -192,7 +159,7 @@ static inline int arp_packet_match(const struct arphdr *arphdr, } /* Look for ifname matches. */ - ret = ifname_compare(indev, arpinfo->iniface, arpinfo->iniface_mask); + ret = ifname_compare_all(indev, arpinfo->iniface, arpinfo->iniface_mask); if (FWINV(ret != 0, ARPT_INV_VIA_IN)) { dprintf("VIA in mismatch (%s vs %s).%s\n", @@ -201,7 +168,7 @@ static inline int arp_packet_match(const struct arphdr *arphdr, return 0; } - ret = ifname_compare(outdev, arpinfo->outiface, arpinfo->outiface_mask); + ret = ifname_compare_all(outdev, arpinfo->outiface, arpinfo->outiface_mask); if (FWINV(ret != 0, ARPT_INV_VIA_OUT)) { dprintf("VIA out mismatch (%s vs %s).%s\n",
arp_tables.c has a 16bit aligment ifname_compare(), factor it out to use it for all tables. Signed-off-by: Richard Weinberger <richard@nod.at> --- include/linux/netfilter/x_tables.h | 25 ++++++++++++++++++------- net/ipv4/netfilter/arp_tables.c | 37 ++----------------------------------- 2 files changed, 20 insertions(+), 42 deletions(-)