]>
Commit | Line | Data |
---|---|---|
2cb7cef9 BS |
1 | Subject: Use state_mutex for switch_log locking, and prevent multiple openers |
2 | From: Jeremy Kerr <jk@ozlabs.org> | |
3 | References: 447133 - LTC50070 | |
4 | ||
5 | Currently, we use ctx->mapping_lock and ctx->switch_log->lock for the | |
6 | context switch log. The mapping lock only prevents concurrent open()s, | |
7 | so we require the switch_lock->lock for reads. | |
8 | ||
9 | Since writes to the switch log buffer occur on context switches, we're | |
10 | better off synchronising with the state_mutex, which is held during a | |
11 | switch. Since we're serialised througout the buffer reads and writes, | |
12 | we can use the state mutex to protect open and release too, and | |
13 | can now kfree() the log buffer on release. This allows us to perform | |
14 | the switch log notify without taking any extra locks. | |
15 | ||
16 | Because the buffer is only present while the file is open, we can use | |
17 | it to prevent multiple simultaneous openers. | |
18 | ||
19 | Signed-off-by: Jeremy Kerr <jk@ozlabs.org> | |
20 | Signed-off-by: Olaf Hering <olh@suse.de> | |
21 | --- | |
22 | arch/powerpc/platforms/cell/spufs/file.c | 133 ++++++++++++++++++------------ | |
23 | arch/powerpc/platforms/cell/spufs/run.c | 3 | |
24 | arch/powerpc/platforms/cell/spufs/spufs.h | 1 | |
25 | 3 files changed, 81 insertions(+), 56 deletions(-) | |
26 | ||
27 | --- a/arch/powerpc/platforms/cell/spufs/file.c | |
28 | +++ b/arch/powerpc/platforms/cell/spufs/file.c | |
29 | @@ -2429,38 +2429,48 @@ static inline int spufs_switch_log_avail | |
30 | static int spufs_switch_log_open(struct inode *inode, struct file *file) | |
31 | { | |
32 | struct spu_context *ctx = SPUFS_I(inode)->i_ctx; | |
33 | + int rc; | |
34 | + | |
35 | + rc = spu_acquire(ctx); | |
36 | + if (rc) | |
37 | + return rc; | |
38 | ||
39 | - /* | |
40 | - * We (ab-)use the mapping_lock here because it serves the similar | |
41 | - * purpose for synchronizing open/close elsewhere. Maybe it should | |
42 | - * be renamed eventually. | |
43 | - */ | |
44 | - mutex_lock(&ctx->mapping_lock); | |
45 | if (ctx->switch_log) { | |
46 | - spin_lock(&ctx->switch_log->lock); | |
47 | - ctx->switch_log->head = 0; | |
48 | - ctx->switch_log->tail = 0; | |
49 | - spin_unlock(&ctx->switch_log->lock); | |
50 | - } else { | |
51 | - /* | |
52 | - * We allocate the switch log data structures on first open. | |
53 | - * They will never be free because we assume a context will | |
54 | - * be traced until it goes away. | |
55 | - */ | |
56 | - ctx->switch_log = kzalloc(sizeof(struct switch_log) + | |
57 | - SWITCH_LOG_BUFSIZE * sizeof(struct switch_log_entry), | |
58 | - GFP_KERNEL); | |
59 | - if (!ctx->switch_log) | |
60 | - goto out; | |
61 | - spin_lock_init(&ctx->switch_log->lock); | |
62 | - init_waitqueue_head(&ctx->switch_log->wait); | |
63 | + rc = -EBUSY; | |
64 | + goto out; | |
65 | + } | |
66 | + | |
67 | + ctx->switch_log = kzalloc(sizeof(struct switch_log) + | |
68 | + SWITCH_LOG_BUFSIZE * sizeof(struct switch_log_entry), | |
69 | + GFP_KERNEL); | |
70 | + | |
71 | + if (!ctx->switch_log) { | |
72 | + rc = -ENOMEM; | |
73 | + goto out; | |
74 | } | |
75 | - mutex_unlock(&ctx->mapping_lock); | |
76 | + | |
77 | + init_waitqueue_head(&ctx->switch_log->wait); | |
78 | + rc = 0; | |
79 | + | |
80 | +out: | |
81 | + spu_release(ctx); | |
82 | + return rc; | |
83 | +} | |
84 | + | |
85 | +static int spufs_switch_log_release(struct inode *inode, struct file *file) | |
86 | +{ | |
87 | + struct spu_context *ctx = SPUFS_I(inode)->i_ctx; | |
88 | + int rc; | |
89 | + | |
90 | + rc = spu_acquire(ctx); | |
91 | + if (rc) | |
92 | + return rc; | |
93 | + | |
94 | + kfree(ctx->switch_log); | |
95 | + ctx->switch_log = NULL; | |
96 | + spu_release(ctx); | |
97 | ||
98 | return 0; | |
99 | - out: | |
100 | - mutex_unlock(&ctx->mapping_lock); | |
101 | - return -ENOMEM; | |
102 | } | |
103 | ||
104 | static int switch_log_sprint(struct spu_context *ctx, char *tbuf, int n) | |
105 | @@ -2488,42 +2498,46 @@ static ssize_t spufs_switch_log_read(str | |
106 | if (!buf || len < 0) | |
107 | return -EINVAL; | |
108 | ||
109 | + error = spu_acquire(ctx); | |
110 | + if (error) | |
111 | + return error; | |
112 | + | |
113 | while (cnt < len) { | |
114 | char tbuf[128]; | |
115 | int width; | |
116 | ||
117 | if (file->f_flags & O_NONBLOCK) { | |
118 | - if (spufs_switch_log_used(ctx) <= 0) | |
119 | - return cnt ? cnt : -EAGAIN; | |
120 | + if (spufs_switch_log_used(ctx) == 0) { | |
121 | + error = -EAGAIN; | |
122 | + break; | |
123 | + } | |
124 | } else { | |
125 | - /* Wait for data in buffer */ | |
126 | - error = wait_event_interruptible(ctx->switch_log->wait, | |
127 | + /* spufs_wait will drop the mutex and re-acquire, | |
128 | + * but since we're in read(), the file cannot be | |
129 | + * _released (and so ctx->switch_log is stable). | |
130 | + */ | |
131 | + error = spufs_wait(ctx->switch_log->wait, | |
132 | spufs_switch_log_used(ctx) > 0); | |
133 | + | |
134 | + /* On error, spufs_wait returns without the | |
135 | + * state mutex held */ | |
136 | if (error) | |
137 | - break; | |
138 | + return error; | |
139 | } | |
140 | ||
141 | - spin_lock(&ctx->switch_log->lock); | |
142 | - if (ctx->switch_log->head == ctx->switch_log->tail) { | |
143 | - /* multiple readers race? */ | |
144 | - spin_unlock(&ctx->switch_log->lock); | |
145 | + /* We may have had entries read from underneath us while we | |
146 | + * dropped the mutex in spufs_wait, so re-check */ | |
147 | + if (ctx->switch_log->head == ctx->switch_log->tail) | |
148 | continue; | |
149 | - } | |
150 | ||
151 | width = switch_log_sprint(ctx, tbuf, sizeof(tbuf)); | |
152 | - if (width < len) { | |
153 | + if (width < len) | |
154 | ctx->switch_log->tail = | |
155 | (ctx->switch_log->tail + 1) % | |
156 | SWITCH_LOG_BUFSIZE; | |
157 | - } | |
158 | - | |
159 | - spin_unlock(&ctx->switch_log->lock); | |
160 | - | |
161 | - /* | |
162 | - * If the record is greater than space available return | |
163 | - * partial buffer (so far) | |
164 | - */ | |
165 | - if (width >= len) | |
166 | + else | |
167 | + /* If the record is greater than space available return | |
168 | + * partial buffer (so far) */ | |
169 | break; | |
170 | ||
171 | error = copy_to_user(buf + cnt, tbuf, width); | |
172 | @@ -2532,6 +2546,8 @@ static ssize_t spufs_switch_log_read(str | |
173 | cnt += width; | |
174 | } | |
175 | ||
176 | + spu_release(ctx); | |
177 | + | |
178 | return cnt == 0 ? error : cnt; | |
179 | } | |
180 | ||
181 | @@ -2540,29 +2556,41 @@ static unsigned int spufs_switch_log_pol | |
182 | struct inode *inode = file->f_path.dentry->d_inode; | |
183 | struct spu_context *ctx = SPUFS_I(inode)->i_ctx; | |
184 | unsigned int mask = 0; | |
185 | + int rc; | |
186 | ||
187 | poll_wait(file, &ctx->switch_log->wait, wait); | |
188 | ||
189 | + rc = spu_acquire(ctx); | |
190 | + if (rc) | |
191 | + return rc; | |
192 | + | |
193 | if (spufs_switch_log_used(ctx) > 0) | |
194 | mask |= POLLIN; | |
195 | ||
196 | + spu_release(ctx); | |
197 | + | |
198 | return mask; | |
199 | } | |
200 | ||
201 | static const struct file_operations spufs_switch_log_fops = { | |
202 | - .owner = THIS_MODULE, | |
203 | - .open = spufs_switch_log_open, | |
204 | - .read = spufs_switch_log_read, | |
205 | - .poll = spufs_switch_log_poll, | |
206 | + .owner = THIS_MODULE, | |
207 | + .open = spufs_switch_log_open, | |
208 | + .read = spufs_switch_log_read, | |
209 | + .poll = spufs_switch_log_poll, | |
210 | + .release = spufs_switch_log_release, | |
211 | }; | |
212 | ||
213 | +/** | |
214 | + * Log a context switch event to a switch log reader. | |
215 | + * | |
216 | + * Must be called with ctx->state_mutex held. | |
217 | + */ | |
218 | void spu_switch_log_notify(struct spu *spu, struct spu_context *ctx, | |
219 | u32 type, u32 val) | |
220 | { | |
221 | if (!ctx->switch_log) | |
222 | return; | |
223 | ||
224 | - spin_lock(&ctx->switch_log->lock); | |
225 | if (spufs_switch_log_avail(ctx) > 1) { | |
226 | struct switch_log_entry *p; | |
227 | ||
228 | @@ -2576,7 +2604,6 @@ void spu_switch_log_notify(struct spu *s | |
229 | ctx->switch_log->head = | |
230 | (ctx->switch_log->head + 1) % SWITCH_LOG_BUFSIZE; | |
231 | } | |
232 | - spin_unlock(&ctx->switch_log->lock); | |
233 | ||
234 | wake_up(&ctx->switch_log->wait); | |
235 | } | |
236 | --- a/arch/powerpc/platforms/cell/spufs/run.c | |
237 | +++ b/arch/powerpc/platforms/cell/spufs/run.c | |
238 | @@ -249,6 +249,7 @@ static int spu_run_fini(struct spu_conte | |
239 | ||
240 | spuctx_switch_state(ctx, SPU_UTIL_IDLE_LOADED); | |
241 | clear_bit(SPU_SCHED_SPU_RUN, &ctx->sched_flags); | |
242 | + spu_switch_log_notify(NULL, ctx, SWITCH_LOG_EXIT, *status); | |
243 | spu_release(ctx); | |
244 | ||
245 | if (signal_pending(current)) | |
246 | @@ -417,8 +418,6 @@ long spufs_run_spu(struct spu_context *c | |
247 | ret = spu_run_fini(ctx, npc, &status); | |
248 | spu_yield(ctx); | |
249 | ||
250 | - spu_switch_log_notify(NULL, ctx, SWITCH_LOG_EXIT, status); | |
251 | - | |
252 | if ((status & SPU_STATUS_STOPPED_BY_STOP) && | |
253 | (((status >> SPU_STOP_STATUS_SHIFT) & 0x3f00) == 0x2100)) | |
254 | ctx->stats.libassist++; | |
255 | --- a/arch/powerpc/platforms/cell/spufs/spufs.h | |
256 | +++ b/arch/powerpc/platforms/cell/spufs/spufs.h | |
257 | @@ -65,7 +65,6 @@ enum { | |
258 | }; | |
259 | ||
260 | struct switch_log { | |
261 | - spinlock_t lock; | |
262 | wait_queue_head_t wait; | |
263 | unsigned long head; | |
264 | unsigned long tail; |