Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy()

[Ingo Molnar]

* Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:

> On Fri, Feb 19, 2016 at 08:58:43AM +0100, Ingo Molnar wrote:
> > > I prefer to use my modern console width to display multiple columns of text, 
> > > instead of wasting it to display mostly whitespace. Therefore I still very much 
> > > prefer ~80 char wide code.
> > 
> > Btw., the main reason I hate the col80 limit is that I see such patches 
> > frequently:
> > 
> >  void pcibios_add_bus(struct pci_bus *bus)
> >  {
> > +#ifdef CONFIG_DMI
> > +       const struct dmi_device *dmi;
> > +       struct dmi_dev_onboard *dslot;
> > +       char sname[128];
> > +
> > +       dmi = NULL;
> > +       while ((dmi = dmi_find_device(DMI_DEV_TYPE_DEV_SLOT,
> > +                                     NULL, dmi)) != NULL) {
> > +               dslot = dmi->device_data;
> > +               if (dslot->segment == pci_domain_nr(bus) &&
> > +                   dslot->bus == bus->number) {
> > +                       dev_info(&bus->dev, "Found SMBIOS Slot %s\n",
> > +                                dslot->dev.name);
> > +                       snprintf(sname, sizeof(sname), "%s-%d",
> > +                                dslot->dev.name,
> > +                                dslot->instance);
> > +                       pci_create_slot(bus, dslot->devfn,
> > +                                       sname, NULL);
> > +               }
> > +       }
> > +#endif
> >         acpi_pci_add_bus(bus);
> > 
> > Which gobbledygook has 6 (!) col80 artifacts - and it's a pretty straightforward 
> > piece of code with just 2 levels of indentation.
> > 
> > It is IMHO much more readable in the following form:
> > 
> >  void pcibios_add_bus(struct pci_bus *bus)
> >  {
> >  #ifdef CONFIG_DMI
> >         const struct dmi_device *dmi;
> >         struct dmi_dev_onboard *dslot;
> >         char sname[128];
> > 
> >         dmi = NULL;
> >         while ((dmi = dmi_find_device(DMI_DEV_TYPE_DEV_SLOT, NULL, dmi)) != NULL) {
> >                 dslot = dmi->device_data;
> >                 if (dslot->segment == pci_domain_nr(bus) && dslot->bus == bus->number) {
> >                         dev_info(&bus->dev, "Found SMBIOS Slot %s\n", dslot->dev.name);
> >                         snprintf(sname, sizeof(sname), "%s-%d", dslot->dev.name, dslot->instance);
> >                         pci_create_slot(bus, dslot->devfn, sname, NULL);
> >                 }
> >         }
> >  #endif
> >         acpi_pci_add_bus(bus);
> > 
> > BYMMV.
> 
> So I mostly agree, although your example does wrap on my normal display
> width. The thing is though, we have to have a limit, otherwise people
> will completely let loose and we'll end up with lines >200 chars wide
> (I've worked on code like that in the past, and its a right pain).

What I'm arguing for is to be, on average, _stricter_ than col80, but not use the 
absolute width as a metric.

Obviusly we have to have limits (to have a consistent coding style) - but I think 
it should be the level of indentation/nesting that should be the limit and the 
number of arguments to a function, while the absolute character count should be 
relaxed in certain cases (and should be made more strict in others!), such as 
printks and other 'leaf' functionality that has no primary side effects.

I'd be fine with only allowing up to 2-3 levels of nesting in typical code 
situations, and not having silly long names. I'd also maximize function arguments 
at about ~4 parameters for the typical case - anything longer should probably 
organize the parameters into helper structures.

But yeah, I can see the pragmatic power of a 'simple' guideline, such as col80.

> And 80 has so far mostly worked just fine. Its just that people seem
> unable to take guidelines as just than, and instead produce the most
> horrible code just to adhere to checkpatch or whatnot.

I think it should be made _stricter_ in many cases.

I.e. col80 is too easy to work around and is routinely worked around in 80% of the 
patches I get.

> And I'd much rather have an extra column of code than waste a lot of
> screen-estate to display mostly whitespace.

So I think that with my proposed rule we'd mostly have much shorter code than 80 
colums, with a few cases (such as printk() lines) where _you_ could easily accept 
automatic, editor generated line wraps instead of forcing the ugliness of manual 
line breaks on everyone else...

The fact is that in almost every patch I receieve these days I see col80 artifacts 
that could be avoided with better code structure - or that would look better if 
they were not manually line-broken.

I.e. I don't so much argue in favor of making lines longer than 80 cols, I argue 
against 'col80 line breaks' that are an easy workaround around the col80 rule.

> Also, this 'artificial' limit on indentation level does in general encourage 
> people to write saner code.

But that's not what we have - col80 is in fact too permissive when it comes to 
actual code complexity!

Let me give a random example - took me 20 seconds to find in kernel/*.c: 
kernel/cgroup.c's cgroup_subtree_control_write():

 - the function is way too big with 230+ lines - it should be split into 2-4
   helper functions.

 - the deepest C indentation it has is too much: 4 levels

 - the function has 11+ 'col80 artifacts' that I counted

 - the function has similar looking code patterns repeated over it

 - the control flow is messy at places - goto patterns mixed with open coded 
   unlock sequences.

 - some logic is completely undocumented - for example can you tell at a glance 
   what the first for_each_subsys() loop does? If it was in a helper function it 
   would be self-documenting to a large degree.

and it's a piece of code that is completely col80 compliant while it has obvious 
problems.

Most of the problems in this function would go away with two relatively simple 
rules, which are in fact stricter than col80 limits:

 - not going over 3 levels deep of nesting.

 - not allowing 'manual line breaks' for non-trivial functionality. This rule
   would flag ugly pieces of code like:

                cgroup_for_each_live_child(child, cgrp) {
                        if (css_enable & (1 << ssid))
                                ret = create_css(child, ss,
                                        cgrp->subtree_control & (1 << ssid));
                        else
                                ret = css_populate_dir(cgroup_css(child, ss),
                                                       NULL);
                        if (ret)
                                goto err_undo_css;
                }

these two rules would IMHO automatically limit the complexity of many functions 
that are too complex today.

Thanks,

	Ingo