Re: USB: hub: Delete an error message for a failed memory allocation in usb_hub_clear_tt_buffer()

Re: USB: hub: Delete an error message for a failed memory allocation in usb_hub_clear_tt_buffer()

[SF Markus Elfring]

> The system will come to a grinding halt anyway if it can't allocate 24 or 40 bytes.

Maybe.


> Which is BTW more or less the amount of memory saved by killing
> the useless (error) message.

Would you dare to resend this update suggestion after such a view?

Regards,
Markus

Re: USB: hub: Delete an error message for a failed memory allocation in usb_hub_clear_tt_buffer()

[Geert Uytterhoeven]

Hi Markus,

On Wed, Dec 6, 2017 at 6:51 PM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
>> The system will come to a grinding halt anyway if it can't allocate 24 or 40 bytes.
>
> Maybe.

Since you've been sending zillions of patches for this, perhaps the time
is ripe to actually try to trigger this, and see what happens?

>> Which is BTW more or less the amount of memory saved by killing
>> the useless (error) message.
>
> Would you dare to resend this update suggestion after such a view?

Of course. That was implied.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: USB: hub: Delete an error message for a failed memory allocation in usb_hub_clear_tt_buffer()

[SF Markus Elfring]

>>> Does the existing memory allocation error message include the 
>>> &udev->dev device name and driver name?  If it doesn't, there will be 
>>> no way for the user to tell that the error message is related to the 
>>> device failure.
>>
>> No, but the effect is similar.
>>
>> OOM does a dump_stack() so this function's call tree is shown.
> 
> A call stack doesn't tell you which device was being handled.

Do you find a default Linux allocation failure report insufficient then?

Would you like to to achieve that the requested information can be determined
from a backtrace?

Regards,
Markus

Re: USB: hub: Delete an error message for a failed memory allocation in usb_hub_clear_tt_buffer()

[Alan Stern]

On Wed, 6 Dec 2017, SF Markus Elfring wrote:

> >>> Does the existing memory allocation error message include the 
> >>> &udev->dev device name and driver name?  If it doesn't, there will be 
> >>> no way for the user to tell that the error message is related to the 
> >>> device failure.
> >>
> >> No, but the effect is similar.
> >>
> >> OOM does a dump_stack() so this function's call tree is shown.
> > 
> > A call stack doesn't tell you which device was being handled.
> 
> Do you find a default Linux allocation failure report insufficient then?
> 
> Would you like to to achieve that the requested information can be determined
> from a backtrace?

It is not practical to do this.  The memory allocation routines do not 
for what purpose the memory is being allocated; hence when a failure 
occurs they cannot tell what device (or other part of the system) will 
be affected.

That's why we have a secondary error message.

Alan Stern

Re: USB: hub: Delete an error message for a failed memory allocation in usb_hub_clear_tt_buffer()

[Geert Uytterhoeven]

Hi Alan,

On Wed, Dec 6, 2017 at 11:02 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Wed, 6 Dec 2017, SF Markus Elfring wrote:
>> >>> Does the existing memory allocation error message include the
>> >>> &udev->dev device name and driver name?  If it doesn't, there will be
>> >>> no way for the user to tell that the error message is related to the
>> >>> device failure.
>> >>
>> >> No, but the effect is similar.
>> >>
>> >> OOM does a dump_stack() so this function's call tree is shown.
>> >
>> > A call stack doesn't tell you which device was being handled.
>>
>> Do you find a default Linux allocation failure report insufficient then?
>>
>> Would you like to to achieve that the requested information can be determined
>> from a backtrace?
>
> It is not practical to do this.  The memory allocation routines do not
> for what purpose the memory is being allocated; hence when a failure
> occurs they cannot tell what device (or other part of the system) will
> be affected.

If even allocation of 24 bytes fails, lots of other devices and other parts of
the system will start failing really soon...

> That's why we have a secondary error message.

... and the secondary error message would still be useless.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: USB: hub: Delete an error message for a failed memory allocation in usb_hub_clear_tt_buffer()

[Dan Carpenter]

On Thu, Dec 07, 2017 at 08:40:07AM +0100, Geert Uytterhoeven wrote:
> Hi Alan,
> 
> On Wed, Dec 6, 2017 at 11:02 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Wed, 6 Dec 2017, SF Markus Elfring wrote:
> >> >>> Does the existing memory allocation error message include the
> >> >>> &udev->dev device name and driver name?  If it doesn't, there will be
> >> >>> no way for the user to tell that the error message is related to the
> >> >>> device failure.
> >> >>
> >> >> No, but the effect is similar.
> >> >>
> >> >> OOM does a dump_stack() so this function's call tree is shown.
> >> >
> >> > A call stack doesn't tell you which device was being handled.
> >>
> >> Do you find a default Linux allocation failure report insufficient then?
> >>
> >> Would you like to to achieve that the requested information can be determined
> >> from a backtrace?
> >
> > It is not practical to do this.  The memory allocation routines do not
> > for what purpose the memory is being allocated; hence when a failure
> > occurs they cannot tell what device (or other part of the system) will
> > be affected.
> 
> If even allocation of 24 bytes fails, lots of other devices and other parts of
> the system will start failing really soon...
> 

Small allocations never fail in the current kernel.

regards,
dan carpenter

Re: USB: hub: Delete an error message for a failed memory allocation in usb_hub_clear_tt_buffer()

[Geert Uytterhoeven]

Hi Dan,

On Thu, Dec 7, 2017 at 9:35 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Thu, Dec 07, 2017 at 08:40:07AM +0100, Geert Uytterhoeven wrote:
>> On Wed, Dec 6, 2017 at 11:02 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
>> > On Wed, 6 Dec 2017, SF Markus Elfring wrote:
>> >> >>> Does the existing memory allocation error message include the
>> >> >>> &udev->dev device name and driver name?  If it doesn't, there will be
>> >> >>> no way for the user to tell that the error message is related to the
>> >> >>> device failure.
>> >> >>
>> >> >> No, but the effect is similar.
>> >> >>
>> >> >> OOM does a dump_stack() so this function's call tree is shown.
>> >> >
>> >> > A call stack doesn't tell you which device was being handled.
>> >>
>> >> Do you find a default Linux allocation failure report insufficient then?
>> >>
>> >> Would you like to to achieve that the requested information can be determined
>> >> from a backtrace?
>> >
>> > It is not practical to do this.  The memory allocation routines do not
>> > for what purpose the memory is being allocated; hence when a failure
>> > occurs they cannot tell what device (or other part of the system) will
>> > be affected.
>>
>> If even allocation of 24 bytes fails, lots of other devices and other parts of
>> the system will start failing really soon...
>
> Small allocations never fail in the current kernel.

A few comments (this is in response to a patch from Markus, so there have
to be lots of questions and uncertainties ;-)
1. In the current kernel. What about the future?
2. If a small allocation cannot fail, what happens if the small memory slab
   is exhausted? A new page must be allocated, which will trigger an OOM,
   and some other part of the system will be killed and fail.
3. This driver uses GFP_ATOMIC, is that guaranteed to succeed? I think not.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: USB: hub: Delete an error message for a failed memory allocation in usb_hub_clear_tt_buffer()

[Greg Kroah-Hartman]

On Thu, Dec 07, 2017 at 09:45:38AM +0100, Geert Uytterhoeven wrote:
> A few comments (this is in response to a patch from Markus, so there have
> to be lots of questions and uncertainties ;-)

Note, because of the patch author, I'm not applying it no matter what,
so this discussion is really going nowhere.

greg k-h

Re: USB: hub: Delete an error message for a failed memory allocation in usb_hub_clear_tt_buffer()

[Oliver Neukum]

Am Donnerstag, den 07.12.2017, 09:56 +0100 schrieb Greg Kroah-Hartman:
> On Thu, Dec 07, 2017 at 09:45:38AM +0100, Geert Uytterhoeven wrote:
> > 
> > A few comments (this is in response to a patch from Markus, so there have
> > to be lots of questions and uncertainties ;-)
> 
> Note, because of the patch author, I'm not applying it no matter what,
> so this discussion is really going nowhere.

We really need to come to a consensus on that question.
If those additional messages really are redundant, we could save
memory deleting them. Or alternatively, if they are not and we need to
know which device is hit, we should centralize that reporting
and tell the VM subsystem not the dump a stack trace.

	Regards
		Oliver

Re: USB: hub: Delete an error message for a failed memory allocation in usb_hub_clear_tt_buffer()

[Dan Carpenter]

On Thu, Dec 07, 2017 at 09:45:38AM +0100, Geert Uytterhoeven wrote:
> >
> > Small allocations never fail in the current kernel.
> 
> A few comments (this is in response to a patch from Markus, so there have
> to be lots of questions and uncertainties ;-)
> 1. In the current kernel. What about the future?

Right.  No one can predict.  And the small allocations don't fail rule
causes some problems.

> 2. If a small allocation cannot fail, what happens if the small memory slab
>    is exhausted? A new page must be allocated, which will trigger an OOM,
>    and some other part of the system will be killed and fail.

Right.


> 3. This driver uses GFP_ATOMIC, is that guaranteed to succeed? I think not.
> 

Right again.  I was missing the first email in the thread because of my
email filters so I didn't see this was atomic.

regards,
dan carpenter

Re: USB: hub: Delete an error message for a failed memory allocation in usb_hub_clear_tt_buffer()

[SF Markus Elfring]

>>>> OOM does a dump_stack() so this function's call tree is shown.
>>>
>>> A call stack doesn't tell you which device was being handled.
>>
>> Do you find a default Linux allocation failure report insufficient then?
>>
>> Would you like to to achieve that the requested information can be determined
>> from a backtrace?
> 
> It is not practical to do this.

I imagine that this depends on details if a backtrace could eventually
be configured for your specific needs.


> The memory allocation routines do not for what purpose
> the memory is being allocated;

Do you want an improved accounting for these purposes?


> hence when a failure occurs they cannot tell what device
> (or other part of the system) will be affected.

I know that other programs can provide dumps for function call
stacks where the parameters which were passed in previous calls
could be decoded to some degree.


> That's why we have a secondary error message.

I am curious on how the relevance of such messages will be interpreted
by other developers in this software area.

Regards,
Markus

Re: USB: hub: Delete an error message for a failed memory allocation in usb_hub_clear_tt_buffer()

[Alan Stern]

On Thu, 7 Dec 2017, Geert Uytterhoeven wrote:

> Hi Alan,
> 
> On Wed, Dec 6, 2017 at 11:02 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Wed, 6 Dec 2017, SF Markus Elfring wrote:
> >> >>> Does the existing memory allocation error message include the
> >> >>> &udev->dev device name and driver name?  If it doesn't, there will be
> >> >>> no way for the user to tell that the error message is related to the
> >> >>> device failure.
> >> >>
> >> >> No, but the effect is similar.
> >> >>
> >> >> OOM does a dump_stack() so this function's call tree is shown.
> >> >
> >> > A call stack doesn't tell you which device was being handled.
> >>
> >> Do you find a default Linux allocation failure report insufficient then?
> >>
> >> Would you like to to achieve that the requested information can be determined
> >> from a backtrace?
> >
> > It is not practical to do this.  The memory allocation routines do not
> > for what purpose the memory is being allocated; hence when a failure
> > occurs they cannot tell what device (or other part of the system) will
> > be affected.
> 
> If even allocation of 24 bytes fails, lots of other devices and other parts of
> the system will start failing really soon...

In fact, one wonders if the allocation routine's own error message and
stack trace would actually appear anywhere...

> > That's why we have a secondary error message.
> 
> ... and the secondary error message would still be useless.

Well, there is still a difference between GFP_ATOMIC and GFP_KERNEL 
allocations.  Failure of the first doesn't necessarily imply failure of 
the second, so perhaps the system could recover.

The real problem is that the kernel development community doesn't have
a fixed policy on how to handle memory allocation errors.  There are
several possibilities:

	Ignore them on the grounds that they will never happen.
	(Really?  And what is the size limit above which they
	might happen?)

	Ignore them on the grounds that the machine will hang or
	crash in the near future.  (Is this guaranteed?)

	Treat them like other errors: try to press forward (perhaps
	in a degraded mode).

	Treat them like other errors: log an error message and try
	to press forward.

And probably a few more that haven't occurred to me.  No doubt there 
are examples of each at various places in the kernel.  Nobody seems
able to agree on a single course of action.  Maybe not even Linus.

If there was one agreed-upon policy, then we could definitively point 
to old code and say "That's wrong, and here is how it should be fixed."  
But currently this is not possible, and we end up with repetitive 
discussions like this one that aren't of general use.

Alan Stern

Re: USB: hub: Delete an error message for a failed memory allocation in usb_hub_clear_tt_buffer()

[Joe Perches]

On Thu, 2017-12-07 at 10:12 -0500, Alan Stern wrote:
> The real problem is that the kernel development community doesn't have
> a fixed policy on how to handle memory allocation errors.
[]
> If there was one agreed-upon policy, then we could definitively point 
> to old code and say "That's wrong, and here is how it should be fixed."  
> But currently this is not possible, and we end up with repetitive 
> discussions like this one that aren't of general use.

Well stated.

My preferred policy would be to remove all the individual
allocation failure messages and only use the generic
warn_alloc()/dump_stack() mechanism.

Re: USB: hub: Delete an error message for a failed memory allocation in usb_hub_clear_tt_buffer()

[Dan Carpenter]

On Thu, Dec 07, 2017 at 10:12:27AM -0500, Alan Stern wrote:
> The real problem is that the kernel development community doesn't have
> a fixed policy on how to handle memory allocation errors.  There are
> several possibilities:
> 
> 	Ignore them on the grounds that they will never happen.
> 	(Really?  And what is the size limit above which they
> 	might happen?)
> 

It's pretty rare to ignore allocation failures these days.  It causes
static checker warnings.

Sometimes it's accepted for people to ignore errors during boot but
I hate that because how am I supposed to filter out those static checker
warnings?  It's better to pretend that the kernel will still boot
without essential hardware instead of wasting everyone's time who looks
at checker output.

> 	Ignore them on the grounds that the machine will hang or
> 	crash in the near future.  (Is this guaranteed?)

On boot sometimes yes.

> 
> 	Treat them like other errors: try to press forward (perhaps
> 	in a degraded mode).
> 
> 	Treat them like other errors: log an error message and try
> 	to press forward.
> 

The standard is to treat them like errors and try press forward in a
degraded mode but don't print a message.  Checkpatch.pl complains if you
print a warning for allocation failures.  A lot of low level functions
handle their own messages pretty well but especially kmalloc() does.

I also have a special static checker warning for when people do:

	foo = alloc();
	BUG_ON(!foo);

People do that occasionally but fortunately it's pretty rare.  10 years
ago that's how btrfs did error handling, but now there are only 4 of
these still remaining in btrfs.

regards,
dan carpenter

Re: USB: hub: Delete an error message for a failed memory allocation in usb_hub_clear_tt_buffer()

[Alan Stern]

On Thu, 7 Dec 2017, Dan Carpenter wrote:

> On Thu, Dec 07, 2017 at 10:12:27AM -0500, Alan Stern wrote:
> > The real problem is that the kernel development community doesn't have
> > a fixed policy on how to handle memory allocation errors.  There are
> > several possibilities:
> > 
> > 	Ignore them on the grounds that they will never happen.
> > 	(Really?  And what is the size limit above which they
> > 	might happen?)
> > 
> 
> It's pretty rare to ignore allocation failures these days.  It causes
> static checker warnings.
> 
> Sometimes it's accepted for people to ignore errors during boot but
> I hate that because how am I supposed to filter out those static checker
> warnings?  It's better to pretend that the kernel will still boot
> without essential hardware instead of wasting everyone's time who looks
> at checker output.
> 
> > 	Ignore them on the grounds that the machine will hang or
> > 	crash in the near future.  (Is this guaranteed?)
> 
> On boot sometimes yes.
> 
> > 
> > 	Treat them like other errors: try to press forward (perhaps
> > 	in a degraded mode).
> > 
> > 	Treat them like other errors: log an error message and try
> > 	to press forward.
> > 
> 
> The standard is to treat them like errors and try press forward in a
> degraded mode but don't print a message.  Checkpatch.pl complains if you
> print a warning for allocation failures.  A lot of low level functions
> handle their own messages pretty well but especially kmalloc() does.

Which brings us back to my original objection.  If an allocation 
failure has localized effects, but the low-level warning is unable to 
specify what will be affected, how is the user supposed to connect the 
effect to the cause?

Alan Stern

> 
> I also have a special static checker warning for when people do:
> 
> 	foo = alloc();
> 	BUG_ON(!foo);
> 
> People do that occasionally but fortunately it's pretty rare.  10 years
> ago that's how btrfs did error handling, but now there are only 4 of
> these still remaining in btrfs.
> 
> regards,
> dan carpenter
> 
> 
> 

Re: USB: hub: Delete an error message for a failed memory allocation in usb_hub_clear_tt_buffer()

[Geert Uytterhoeven]

Hi Alan,

On Thu, Dec 7, 2017 at 10:26 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Thu, 7 Dec 2017, Dan Carpenter wrote:
>> The standard is to treat them like errors and try press forward in a
>> degraded mode but don't print a message.  Checkpatch.pl complains if you
>> print a warning for allocation failures.  A lot of low level functions
>> handle their own messages pretty well but especially kmalloc() does.
>
> Which brings us back to my original objection.  If an allocation
> failure has localized effects, but the low-level warning is unable to
> specify what will be affected, how is the user supposed to connect the
> effect to the cause?

The backtrace would include usb_hub_clear_tt_buffer, so the user will
know USB is affected.
Note that the cause of the memory exhaustion is usually not the caller.
Unless it requests a real big block of memory, in which case that will be
mentioned in the backtrace, too.

In this particular case, the driver uses GFP_ATOMIC, so allocation failures
aren't that rare, and I think the driver should be prepared for that, and try
to recover gracefully.

The comment

                /* FIXME recover somehow ... RESET_TT? */

makes me think it isn't.

As this is a pretty small allocation, perhaps it can be done beforehand, without
GFP_ATOMIC?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: USB: hub: Delete an error message for a failed memory allocation in usb_hub_clear_tt_buffer()

[Dan Carpenter]

On Wed, Dec 06, 2017 at 09:20:05PM +0100, Geert Uytterhoeven wrote:
> Hi Markus,
> 
> On Wed, Dec 6, 2017 at 6:51 PM, SF Markus Elfring
> <elfring@users.sourceforge.net> wrote:
> >> The system will come to a grinding halt anyway if it can't allocate 24 or 40 bytes.
> >
> > Maybe.
> 
> Since you've been sending zillions of patches for this, perhaps the time
> is ripe to actually try to trigger this, and see what happens?
> 
> >> Which is BTW more or less the amount of memory saved by killing
> >> the useless (error) message.
> >
> > Would you dare to resend this update suggestion after such a view?
> 
> Of course. That was implied.
> 

No.  Greg maintains USB and he's has blocked Markus, because he never
listens to feedback but instead just repsonds that he has a different
opinion.

regards,
dan carpenter

Re: USB: hub: Delete an error message for a failed memory allocation in usb_hub_clear_tt_buffer()

[Alan Stern]

On Fri, 8 Dec 2017, Geert Uytterhoeven wrote:

> Hi Alan,
> 
> On Thu, Dec 7, 2017 at 10:26 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Thu, 7 Dec 2017, Dan Carpenter wrote:
> >> The standard is to treat them like errors and try press forward in a
> >> degraded mode but don't print a message.  Checkpatch.pl complains if you
> >> print a warning for allocation failures.  A lot of low level functions
> >> handle their own messages pretty well but especially kmalloc() does.
> >
> > Which brings us back to my original objection.  If an allocation
> > failure has localized effects, but the low-level warning is unable to
> > specify what will be affected, how is the user supposed to connect the
> > effect to the cause?
> 
> The backtrace would include usb_hub_clear_tt_buffer, so the user will
> know USB is affected.
> Note that the cause of the memory exhaustion is usually not the caller.
> Unless it requests a real big block of memory, in which case that will be
> mentioned in the backtrace, too.
> 
> In this particular case, the driver uses GFP_ATOMIC, so allocation failures
> aren't that rare, and I think the driver should be prepared for that, and try
> to recover gracefully.
> 
> The comment
> 
>                 /* FIXME recover somehow ... RESET_TT? */
> 
> makes me think it isn't.
> 
> As this is a pretty small allocation, perhaps it can be done beforehand, without
> GFP_ATOMIC?

I can't see how to make that work.  We don't know beforehand how many
structures will be needed at any time.

Alan Stern