Re: [RFC v2 2/2] scsi: add rate limit to scsi sense code uevent

2017-06-08 Thread Song Liu 

> On Jun 6, 2017, at 6:22 AM, Ewan D. Milne  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 
>> ---
>> 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,
>>>sense_event_filter))
>>  return;
>>  evt = sdev_evt_alloc(SDEV_EVT_SCSI_SENSE, GFP_ATOMIC);
>> +
>> +time_ns = ktime_to_ns(ktime_get());
>> +spin_lock_irqsave(>latest_event_lock, flags);
>> +if (time_ns - sdev->latest_event_times[sdev->latest_event_idx] <
>> +NSEC_PER_SEC) {
>> +spin_unlock_irqrestore(>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

Re: [RFC v2 2/2] scsi: add rate limit to scsi sense code uevent

2017-06-05 Thread Ewan D. Milne 
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 
> ---
>  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,
> >sense_event_filter))
>   return;
>   evt = sdev_evt_alloc(SDEV_EVT_SCSI_SENSE, GFP_ATOMIC);
> +
> + time_ns = ktime_to_ns(ktime_get());
> + spin_lock_irqsave(>latest_event_lock, flags);
> + if (time_ns - sdev->latest_event_times[sdev->latest_event_idx] <
> + NSEC_PER_SEC) {
> + spin_unlock_irqrestore(>latest_event_lock, flags);
> + return;

You have an allocated evt here, so if you return here you will leak
kernel memory.

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.

-Ewan

> + }
> + sdev->latest_event_times[sdev->latest_event_idx] = time_ns;
> + sdev->latest_event_idx = (sdev->latest_event_idx + 1) %
> + MAX_SENSE_EVENT_PER_SECOND;
> + spin_unlock_irqrestore(>latest_event_lock, flags);
> +
>   if (!evt)
>   return;
>  
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index 6f7128f..67b3f71 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -301,6 +301,10 @@ static struct scsi_device *scsi_alloc_sdev(struct 
> scsi_target *starget,
>   }
>   }
>  
> +#ifdef CONFIG_SCSI_SENSE_UEVENT
> + spin_lock_init(>latest_event_lock);
> +#endif
> +
>   return sdev;
>  
>  out_device_destroy:
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index 5b6f06d..9217dae 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -214,12 +214,22 @@ struct scsi_device {
>   atomic_t ioerr_cnt;
>  
>  #ifdef CONFIG_SCSI_SENSE_UEVENT
> +#define MAX_SENSE_EVENT_PER_SECOND   16
>   /*
>* filter of sense code uevent
>* setting bit X (0x00 - 0x0e) of sense_event_filter enables
>* uevent for sense key X
>*/
> - unsigned long   sense_event_filter;
> + unsigned long   sense_event_filter;
> + /*
> +  * To rate limit uevents to MAX_SENSE_EVENT_PER_SECOND, we track
> +  * nano second time of MAX_SENSE_EVENT_PER_SECOND most recent
> +  * events. If there are already MAX_SENSE_EVENT_PER_SECOND in the
> +  * past seconds, new event is dropped.
> +  */
> + u64 latest_event_times[MAX_SENSE_EVENT_PER_SECOND];
> + int latest_event_idx;
> + spinlock_t  latest_event_lock;
>  #endif
>  
>   struct device   sdev_gendev,

[RFC v2 2/2] scsi: add rate limit to scsi sense code uevent

2017-05-18 Thread Song Liu 
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 
---
 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,
  >sense_event_filter))
return;
evt = sdev_evt_alloc(SDEV_EVT_SCSI_SENSE, GFP_ATOMIC);
+
+   time_ns = ktime_to_ns(ktime_get());
+   spin_lock_irqsave(>latest_event_lock, flags);
+   if (time_ns - sdev->latest_event_times[sdev->latest_event_idx] <
+   NSEC_PER_SEC) {
+   spin_unlock_irqrestore(>latest_event_lock, flags);
+   return;
+   }
+   sdev->latest_event_times[sdev->latest_event_idx] = time_ns;
+   sdev->latest_event_idx = (sdev->latest_event_idx + 1) %
+   MAX_SENSE_EVENT_PER_SECOND;
+   spin_unlock_irqrestore(>latest_event_lock, flags);
+
if (!evt)
return;
 
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 6f7128f..67b3f71 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -301,6 +301,10 @@ static struct scsi_device *scsi_alloc_sdev(struct 
scsi_target *starget,
}
}
 
+#ifdef CONFIG_SCSI_SENSE_UEVENT
+   spin_lock_init(>latest_event_lock);
+#endif
+
return sdev;
 
 out_device_destroy:
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 5b6f06d..9217dae 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -214,12 +214,22 @@ struct scsi_device {
atomic_t ioerr_cnt;
 
 #ifdef CONFIG_SCSI_SENSE_UEVENT
+#define MAX_SENSE_EVENT_PER_SECOND 16
/*
 * filter of sense code uevent
 * setting bit X (0x00 - 0x0e) of sense_event_filter enables
 * uevent for sense key X
 */
-   unsigned long   sense_event_filter;
+   unsigned long   sense_event_filter;
+   /*
+* To rate limit uevents to MAX_SENSE_EVENT_PER_SECOND, we track
+* nano second time of MAX_SENSE_EVENT_PER_SECOND most recent
+* events. If there are already MAX_SENSE_EVENT_PER_SECOND in the
+* past seconds, new event is dropped.
+*/
+   u64 latest_event_times[MAX_SENSE_EVENT_PER_SECOND];
+   int latest_event_idx;
+   spinlock_t  latest_event_lock;
 #endif
 
struct device   sdev_gendev,
-- 
2.9.3