> On Jun 6, 2017, at 6:22 AM, Ewan D. Milne <emi...@redhat.com> wrote:
>
> On Thu, 2017-05-18 at 11:14 -0700, Song Liu wrote:
>> This patch adds rate limits to SCSI sense code uevets. Currently,
>> the rate limit is hard-coded to 16 events per second.
>>
>> The code tracks nano second time of latest 16 events in a circular
>> buffer. When a new event arrives, the time is compared against the
>> latest time in the buffer. If the difference is smaller than one
>> second, the new event is dropped.
>>
>> Signed-off-by: Song Liu <songliubrav...@fb.com>
>> ---
>> drivers/scsi/scsi_error.c | 15 +++++++++++++++
>> drivers/scsi/scsi_scan.c | 4 ++++
>> include/scsi/scsi_device.h | 12 +++++++++++-
>> 3 files changed, 30 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
>> index a49c63a..91e6b0a 100644
>> --- a/drivers/scsi/scsi_error.c
>> +++ b/drivers/scsi/scsi_error.c
>> @@ -436,11 +436,26 @@ static void scsi_send_sense_uevent(struct scsi_device
>> *sdev,
>> #ifdef CONFIG_SCSI_SENSE_UEVENT
>> struct scsi_event *evt;
>> unsigned char sb_len;
>> + unsigned long flags;
>> + u64 time_ns;
>>
>> if (!test_bit(sshdr->sense_key & 0xf,
>> &sdev->sense_event_filter))
>> return;
>> evt = sdev_evt_alloc(SDEV_EVT_SCSI_SENSE, GFP_ATOMIC);
>> +
>> + time_ns = ktime_to_ns(ktime_get());
>> + spin_lock_irqsave(&sdev->latest_event_lock, flags);
>> + if (time_ns - sdev->latest_event_times[sdev->latest_event_idx] <
>> + NSEC_PER_SEC) {
>> + spin_unlock_irqrestore(&sdev->latest_event_lock, flags);
>> + return;
>
> You have an allocated evt here, so if you return here you will leak
> kernel memory.
I will fix this in next version.
>
> This code appears to be rate limiting per-sdev. You have to remember
> that on a large system, there could be thousands of these devices.
> You could easily end up generating 10s or 100s of thousands of events
> per second, in a very short amount of time under certain failure
> conditions.
>
> The udev event mechanism can realistically only process a few
> hundred events per second before it starts getting behind, due to
> the rule processing engine (the rules in userspace).
>
> You also have to consider that if you clog up udev with a lot of
> your events, you affect event processing for other events.
>
Yeah, this is great point. Would Scsi_Host level rate limiting make
sense for this type of cases? Or shall I add a global rate limit?
Thanks,
Song
