On Fri, 2006-03-24 at 09:21 +0800, Yi Yang wrote:
> Matt Helsley wrote:
>
> Thanks for Matt's careful review. I'll follow your advices to modify it
> and new version will be released soon.
> > On Wed, 2006-03-22 at 22:58 +0800, Yi Yang wrote:
> >
<snip>
> >> +int __raise_fsevent(const char * oldname, const char * newname, u32 mask)
> >>
> >
> > The return value of this function does not appear to be used.
> >
> If some modules want to use it to transfer file system events reliably,
> the return value will be very valuble,
> because the caller can retry the transfer until it successes.
Fair enough, though I hope you'll return -EFOO rather than -1, -2,...
<snip>
> >> + int namelen = 0;
> >> + static unsigned long last = 0;
> >> + static int fsevent_sum = 0;
> >>
> >
> > Yuck, static local variables. IMHO these should be globals. It would
> > make the fact they aren't protected from concurrent access more obvious
> > (see below).
> >
> Yes, they should be global.
> >
> >> + if (atomic_read(&cn_fs_event_listeners) < 1)
> >> + return 0;
> >> +
> >> + if (jiffies - last <= fsevent_ratelimit) {
> >> + if (fsevent_sum > fsevent_burst_limit) {
> >> + return -1;
> >>
> >
> > OK, so you're rate limiting the events. Shouldn't you still boost the
> > sequence number so that userspace knows some events got dropped? Also
> > perhaps you can find an appropriate error to return instead of -1.
> >
> Good idea.
> >
> >> + }
> >>
> >
> > remove unecessary braces
> >
> >
> >> + fsevent_sum++;
> >>
> >
> > Looks racy to me -- what's protecting fsevent_sum from access by
> > multiple cpus?
> >
> This just is used to limit event rate when the user space application
> leads to an unlimited events loop.
> so it mustn't be precise, I used spinlock originally, but Andrew thinks
> lock overhead is big, inotify has led to
> some frustrating lock overhead, so I decide to remove it, in fact
> fsevent_sum just is the number used to limit rate,
> some race conditions don't effect the rate limit.
OK, I can see why you would want to avoid a spinlock. However spinlocks
are not your only option. For instance you could use the per-cpu idioms
to limit the rate.
I would argue preemption should be disabled around the if-block at the
very least. Suppose your rate limit is 10k calls/sec and you have 4
procs. Each proc has a sequence of three instructions:
load fsevent_sum into register rx (rx <= 1000)
rx++ (rx <= 1001)
store contents of register rx in fsevent_sum (fsevent_sum <= 1001)
Now consider the following sequence of steps:
load fsevent_sum into rx (rx <= 1000)
<preempted>
<3 other processors each manage to increment the sum by 3333 bringing us
to 9999>
<resumed>
rx++ (rx <= 1001)
store contents of rx in fsevent_sum (fsevent_sum <= 1001)
So every processor now thinks it won't exceed the rate limit by
generating more events when in fact we've just exceeded the limit. So,
unless my example is flawed, I think you need to disable preemption
here.
Also, even if you simply disable preemption couldn't this cause the
cache line containing the sum to bounce frequently on large SMP systems?
<snip>
Cheers,
-Matt Helsley
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
