Re: [Intel-gfx] [PATCH v2 0/4] eventfd: simplify signal helpers
On 11/22/23 5:48 AM, Christian Brauner wrote: > Hey everyone, > > This simplifies the eventfd_signal() and eventfd_signal_mask() helpers > significantly. They can be made void and not take any unnecessary > arguments. > > I've added a few more simplifications based on Sean's suggestion. > > Signed-off-by: Christian Brauner > > Changes in v2: > - further simplify helpers > - Link to v1: > https://lore.kernel.org/r/20230713-vfs-eventfd-signal-v1-0-7fda6c5d2...@kernel.org Only oddity I spotted was the kerneldoc, which someone else already brought up. Reviewed-by: Jens Axboe -- Jens Axboe
Re: [Intel-gfx] Public i915 CI shardruns are disabled
On 3/2/21 6:01 PM, Linus Torvalds wrote: > On Tue, Mar 2, 2021 at 4:36 PM Jens Axboe wrote: >> >> Or if you want a pull, just let me know. Have another misc patch to >> flush out anyway that doesn't belong in any of my usual branches. > > Ok, if you have something else pending anyway, let's do that. Send me > the pull request, and I'll take it asap. Done -- Jens Axboe ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] Public i915 CI shardruns are disabled
On 3/2/21 5:15 PM, Jens Axboe wrote: > On 3/2/21 4:56 PM, Linus Torvalds wrote: >> On Tue, Mar 2, 2021 at 3:38 PM Dave Airlie wrote: >>> >>> Looks like Jens saw it at least, he posted this on twitter a few mins >>> ago so I assume it'll be incoming soon. >>> >>> https://git.kernel.dk/cgit/linux-block/commit/?h=swap-fix >> >> Ahh. You use a swap file. This might be the same thing that I think >> the phoronix people hit as ext4 corruption this merge window. >> >> Jens, if that can get confirmed, please send it my way asap.. Thanks, > > Yep, it's the same issue indeed. Was made aware of it after lunch today > and emailed Christoph, but then decided to dig into it myself a few > hours later. Andrew already queued it up I just saw, but I noticed that > that version will break on !CONFIG_HIBERNATION. > > Patch below if you just want to grab it. Or if you want a pull, just let me know. Have another misc patch to flush out anyway that doesn't belong in any of my usual branches. -- Jens Axboe ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] Public i915 CI shardruns are disabled
On 3/2/21 4:56 PM, Linus Torvalds wrote:
> On Tue, Mar 2, 2021 at 3:38 PM Dave Airlie wrote:
>>
>> Looks like Jens saw it at least, he posted this on twitter a few mins
>> ago so I assume it'll be incoming soon.
>>
>> https://git.kernel.dk/cgit/linux-block/commit/?h=swap-fix
>
> Ahh. You use a swap file. This might be the same thing that I think
> the phoronix people hit as ext4 corruption this merge window.
>
> Jens, if that can get confirmed, please send it my way asap.. Thanks,
Yep, it's the same issue indeed. Was made aware of it after lunch today
and emailed Christoph, but then decided to dig into it myself a few
hours later. Andrew already queued it up I just saw, but I noticed that
that version will break on !CONFIG_HIBERNATION.
Patch below if you just want to grab it.
commit e25b1010db005a59727e1ff5f43af889effd31a3
Author: Jens Axboe
Date: Tue Mar 2 14:53:21 2021 -0700
swap: fix swapfile read/write offset
We're not factoring in the start of the file for where to write and
read the swapfile, which leads to very unfortunate side effects of
writing where we should not be...
Fixes: 48d15436fde6 ("mm: remove get_swap_bio")
Signed-off-by: Jens Axboe
diff --git a/include/linux/swap.h b/include/linux/swap.h
index 32f665b1ee85..4cc6ec3bf0ab 100644
--- a/include/linux/swap.h
+++ b/include/linux/swap.h
@@ -485,6 +485,7 @@ struct backing_dev_info;
extern int init_swap_address_space(unsigned int type, unsigned long nr_pages);
extern void exit_swap_address_space(unsigned int type);
extern struct swap_info_struct *get_swap_device(swp_entry_t entry);
+sector_t swap_page_sector(struct page *page);
static inline void put_swap_device(struct swap_info_struct *si)
{
diff --git a/mm/page_io.c b/mm/page_io.c
index 485fa5cca4a2..c493ce9ebcf5 100644
--- a/mm/page_io.c
+++ b/mm/page_io.c
@@ -254,11 +254,6 @@ int swap_writepage(struct page *page, struct
writeback_control *wbc)
return ret;
}
-static sector_t swap_page_sector(struct page *page)
-{
- return (sector_t)__page_file_index(page) << (PAGE_SHIFT - 9);
-}
-
static inline void count_swpout_vm_event(struct page *page)
{
#ifdef CONFIG_TRANSPARENT_HUGEPAGE
diff --git a/mm/swapfile.c b/mm/swapfile.c
index f039745989d2..084a5b9a18e5 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -219,6 +219,19 @@ offset_to_swap_extent(struct swap_info_struct *sis,
unsigned long offset)
BUG();
}
+sector_t swap_page_sector(struct page *page)
+{
+ struct swap_info_struct *sis = page_swap_info(page);
+ struct swap_extent *se;
+ sector_t sector;
+ pgoff_t offset;
+
+ offset = __page_file_index(page);
+ se = offset_to_swap_extent(sis, offset);
+ sector = se->start_block + (offset - se->start_page);
+ return sector << (PAGE_SHIFT - 9);
+}
+
/*
* swap allocation tell device that a cluster of swap can now be discarded,
* to allow the swap device to optimize its wear-levelling.
--
Jens Axboe
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] block: convert tasklets to use new tasklet_setup() API
On 8/19/20 9:24 AM, Allen wrote: >> [...] >>>> Since both threads seem to have petered out, let me suggest in >>>> kernel.h: >>>> >>>> #define cast_out(ptr, container, member) \ >>>> container_of(ptr, typeof(*container), member) >>>> >>>> It does what you want, the argument order is the same as >>>> container_of with the only difference being you name the containing >>>> structure instead of having to specify its type. >>> >>> Not to incessantly bike shed on the naming, but I don't like >>> cast_out, it's not very descriptive. And it has connotations of >>> getting rid of something, which isn't really true. >> >> Um, I thought it was exactly descriptive: you're casting to the outer >> container. I thought about following the C++ dynamic casting style, so >> out_cast(), but that seemed a bit pejorative. What about outer_cast()? >> >>> FWIW, I like the from_ part of the original naming, as it has some >>> clues as to what is being done here. Why not just from_container()? >>> That should immediately tell people what it does without having to >>> look up the implementation, even before this becomes a part of the >>> accepted coding norm. >> >> I'm not opposed to container_from() but it seems a little less >> descriptive than outer_cast() but I don't really care. I always have >> to look up container_of() when I'm using it so this would just be >> another macro of that type ... >> > > So far we have a few which have been suggested as replacement > for from_tasklet() > > - out_cast() or outer_cast() > - from_member(). > - container_from() or from_container() > > from_container() sounds fine, would trimming it a bit work? like from_cont(). I like container_from() the most, since it's the closest to contain_of() which is a well known idiom for years. The lines will already be shorter without the need to specify the struct, so don't like the idea of squeezing container into cont for any of them. For most people, cont is usually short for continue, not container. -- Jens Axboe ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] block: convert tasklets to use new tasklet_setup() API
On 8/19/20 6:11 AM, Greg KH wrote: > On Wed, Aug 19, 2020 at 07:00:53AM -0600, Jens Axboe wrote: >> On 8/18/20 1:00 PM, James Bottomley wrote: >>> On Mon, 2020-08-17 at 13:02 -0700, Jens Axboe wrote: >>>> On 8/17/20 12:48 PM, Kees Cook wrote: >>>>> On Mon, Aug 17, 2020 at 12:44:34PM -0700, Jens Axboe wrote: >>>>>> On 8/17/20 12:29 PM, Kees Cook wrote: >>>>>>> On Mon, Aug 17, 2020 at 06:56:47AM -0700, Jens Axboe wrote: >>>>>>>> On 8/17/20 2:15 AM, Allen Pais wrote: >>>>>>>>> From: Allen Pais >>>>>>>>> >>>>>>>>> In preparation for unconditionally passing the >>>>>>>>> struct tasklet_struct pointer to all tasklet >>>>>>>>> callbacks, switch to using the new tasklet_setup() >>>>>>>>> and from_tasklet() to pass the tasklet pointer explicitly. >>>>>>>> >>>>>>>> Who came up with the idea to add a macro 'from_tasklet' that >>>>>>>> is just container_of? container_of in the code would be >>>>>>>> _much_ more readable, and not leave anyone guessing wtf >>>>>>>> from_tasklet is doing. >>>>>>>> >>>>>>>> I'd fix that up now before everything else goes in... >>>>>>> >>>>>>> As I mentioned in the other thread, I think this makes things >>>>>>> much more readable. It's the same thing that the timer_struct >>>>>>> conversion did (added a container_of wrapper) to avoid the >>>>>>> ever-repeating use of typeof(), long lines, etc. >>>>>> >>>>>> But then it should use a generic name, instead of each sub-system >>>>>> using some random name that makes people look up exactly what it >>>>>> does. I'm not huge fan of the container_of() redundancy, but >>>>>> adding private variants of this doesn't seem like the best way >>>>>> forward. Let's have a generic helper that does this, and use it >>>>>> everywhere. >>>>> >>>>> I'm open to suggestions, but as things stand, these kinds of >>>>> treewide >>>> >>>> On naming? Implementation is just as it stands, from_tasklet() is >>>> totally generic which is why I objected to it. from_member()? Not >>>> great with naming... But I can see this going further and then we'll >>>> suddenly have tons of these. It's not good for readability. >>> >>> Since both threads seem to have petered out, let me suggest in >>> kernel.h: >>> >>> #define cast_out(ptr, container, member) \ >>> container_of(ptr, typeof(*container), member) >>> >>> It does what you want, the argument order is the same as container_of >>> with the only difference being you name the containing structure >>> instead of having to specify its type. >> >> Not to incessantly bike shed on the naming, but I don't like cast_out, >> it's not very descriptive. And it has connotations of getting rid of >> something, which isn't really true. > > I agree, if we want to bike shed, I don't like this color either. > >> FWIW, I like the from_ part of the original naming, as it has some clues >> as to what is being done here. Why not just from_container()? That >> should immediately tell people what it does without having to look up >> the implementation, even before this becomes a part of the accepted >> coding norm. > > Why are people hating on the well-known and used container_of()? > > If you really hate to type the type and want a new macro, what about > 'container_from()'? (noun/verb is nicer to sort symbols by...) > > But really, why is this even needed? container_from() or from_container(), either works just fine for me in terms of naming. I think people are hating on it because it makes for _really_ long lines, and it's arguably cleaner/simpler to just pass in the pointer type instead. Then you end up with lines like this: struct request_queue *q = container_of(work, struct request_queue, requeue_work.work); But I'm not the one that started this addition of from_tasklet(), my objection was adding a private macro for something that should be generic functionality. Hence I think we either need to provide that, or tell the from_tasklet() folks that they should just use container_of(). -- Jens Axboe ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] block: convert tasklets to use new tasklet_setup() API
On 8/18/20 1:00 PM, James Bottomley wrote: > On Mon, 2020-08-17 at 13:02 -0700, Jens Axboe wrote: >> On 8/17/20 12:48 PM, Kees Cook wrote: >>> On Mon, Aug 17, 2020 at 12:44:34PM -0700, Jens Axboe wrote: >>>> On 8/17/20 12:29 PM, Kees Cook wrote: >>>>> On Mon, Aug 17, 2020 at 06:56:47AM -0700, Jens Axboe wrote: >>>>>> On 8/17/20 2:15 AM, Allen Pais wrote: >>>>>>> From: Allen Pais >>>>>>> >>>>>>> In preparation for unconditionally passing the >>>>>>> struct tasklet_struct pointer to all tasklet >>>>>>> callbacks, switch to using the new tasklet_setup() >>>>>>> and from_tasklet() to pass the tasklet pointer explicitly. >>>>>> >>>>>> Who came up with the idea to add a macro 'from_tasklet' that >>>>>> is just container_of? container_of in the code would be >>>>>> _much_ more readable, and not leave anyone guessing wtf >>>>>> from_tasklet is doing. >>>>>> >>>>>> I'd fix that up now before everything else goes in... >>>>> >>>>> As I mentioned in the other thread, I think this makes things >>>>> much more readable. It's the same thing that the timer_struct >>>>> conversion did (added a container_of wrapper) to avoid the >>>>> ever-repeating use of typeof(), long lines, etc. >>>> >>>> But then it should use a generic name, instead of each sub-system >>>> using some random name that makes people look up exactly what it >>>> does. I'm not huge fan of the container_of() redundancy, but >>>> adding private variants of this doesn't seem like the best way >>>> forward. Let's have a generic helper that does this, and use it >>>> everywhere. >>> >>> I'm open to suggestions, but as things stand, these kinds of >>> treewide >> >> On naming? Implementation is just as it stands, from_tasklet() is >> totally generic which is why I objected to it. from_member()? Not >> great with naming... But I can see this going further and then we'll >> suddenly have tons of these. It's not good for readability. > > Since both threads seem to have petered out, let me suggest in > kernel.h: > > #define cast_out(ptr, container, member) \ > container_of(ptr, typeof(*container), member) > > It does what you want, the argument order is the same as container_of > with the only difference being you name the containing structure > instead of having to specify its type. Not to incessantly bike shed on the naming, but I don't like cast_out, it's not very descriptive. And it has connotations of getting rid of something, which isn't really true. FWIW, I like the from_ part of the original naming, as it has some clues as to what is being done here. Why not just from_container()? That should immediately tell people what it does without having to look up the implementation, even before this becomes a part of the accepted coding norm. -- Jens Axboe ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] block: convert tasklets to use new tasklet_setup() API
On 8/17/20 12:48 PM, Kees Cook wrote: > On Mon, Aug 17, 2020 at 12:44:34PM -0700, Jens Axboe wrote: >> On 8/17/20 12:29 PM, Kees Cook wrote: >>> On Mon, Aug 17, 2020 at 06:56:47AM -0700, Jens Axboe wrote: >>>> On 8/17/20 2:15 AM, Allen Pais wrote: >>>>> From: Allen Pais >>>>> >>>>> In preparation for unconditionally passing the >>>>> struct tasklet_struct pointer to all tasklet >>>>> callbacks, switch to using the new tasklet_setup() >>>>> and from_tasklet() to pass the tasklet pointer explicitly. >>>> >>>> Who came up with the idea to add a macro 'from_tasklet' that is just >>>> container_of? container_of in the code would be _much_ more readable, >>>> and not leave anyone guessing wtf from_tasklet is doing. >>>> >>>> I'd fix that up now before everything else goes in... >>> >>> As I mentioned in the other thread, I think this makes things much more >>> readable. It's the same thing that the timer_struct conversion did >>> (added a container_of wrapper) to avoid the ever-repeating use of >>> typeof(), long lines, etc. >> >> But then it should use a generic name, instead of each sub-system using >> some random name that makes people look up exactly what it does. I'm not >> huge fan of the container_of() redundancy, but adding private variants >> of this doesn't seem like the best way forward. Let's have a generic >> helper that does this, and use it everywhere. > > I'm open to suggestions, but as things stand, these kinds of treewide On naming? Implementation is just as it stands, from_tasklet() is totally generic which is why I objected to it. from_member()? Not great with naming... But I can see this going further and then we'll suddenly have tons of these. It's not good for readability. > changes end up getting whole-release delays because of the need to have > the API in place for everyone before patches to do the changes can be > sent to multiple maintainers, etc. Sure, that's always true of treewide changes like that. -- Jens Axboe ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] block: convert tasklets to use new tasklet_setup() API
On 8/17/20 12:29 PM, Kees Cook wrote: > On Mon, Aug 17, 2020 at 06:56:47AM -0700, Jens Axboe wrote: >> On 8/17/20 2:15 AM, Allen Pais wrote: >>> From: Allen Pais >>> >>> In preparation for unconditionally passing the >>> struct tasklet_struct pointer to all tasklet >>> callbacks, switch to using the new tasklet_setup() >>> and from_tasklet() to pass the tasklet pointer explicitly. >> >> Who came up with the idea to add a macro 'from_tasklet' that is just >> container_of? container_of in the code would be _much_ more readable, >> and not leave anyone guessing wtf from_tasklet is doing. >> >> I'd fix that up now before everything else goes in... > > As I mentioned in the other thread, I think this makes things much more > readable. It's the same thing that the timer_struct conversion did > (added a container_of wrapper) to avoid the ever-repeating use of > typeof(), long lines, etc. But then it should use a generic name, instead of each sub-system using some random name that makes people look up exactly what it does. I'm not huge fan of the container_of() redundancy, but adding private variants of this doesn't seem like the best way forward. Let's have a generic helper that does this, and use it everywhere. -- Jens Axboe ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] block: convert tasklets to use new tasklet_setup() API
On 8/17/20 2:15 AM, Allen Pais wrote: > From: Allen Pais > > In preparation for unconditionally passing the > struct tasklet_struct pointer to all tasklet > callbacks, switch to using the new tasklet_setup() > and from_tasklet() to pass the tasklet pointer explicitly. Who came up with the idea to add a macro 'from_tasklet' that is just container_of? container_of in the code would be _much_ more readable, and not leave anyone guessing wtf from_tasklet is doing. I'd fix that up now before everything else goes in... -- Jens Axboe ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] improve use_mm / unuse_mm v2
On 4/15/20 11:31 PM, Christoph Hellwig wrote: > Hi all, > > this series improves the use_mm / unuse_mm interface by better > documenting the assumptions, and my taking the set_fs manipulations > spread over the callers into the core API. > > Changes since v1: > - drop a few patches > - fix a comment typo > - cover the newly merged use_mm/unuse_mm caller in vfio You can add my reviewed-by/tested-by to the patches, passes the io_uring regression tests. -- Jens Axboe ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 02/12] blk: use for_each_if
On 7/12/18 12:45 AM, Joe Perches wrote:
> On Wed, 2018-07-11 at 20:50 +0200, Daniel Vetter wrote:
>> On Wed, Jul 11, 2018 at 8:30 PM, Jens Axboe wrote:
>>> On 7/11/18 10:45 AM, Tejun Heo wrote:
>>>> On Wed, Jul 11, 2018 at 09:40:58AM -0700, Tejun Heo wrote:
>>>>> On Mon, Jul 09, 2018 at 10:36:40AM +0200, Daniel Vetter wrote:
>>>>>> Makes the macros resilient against if {} else {} blocks right
>>>>>> afterwards.
>>>>>>
>>>>>> Signed-off-by: Daniel Vetter
>>>>>> Cc: Tejun Heo
>>>>>> Cc: Jens Axboe
>>>>>> Cc: Shaohua Li
>>>>>> Cc: Kate Stewart
>>>>>> Cc: Greg Kroah-Hartman
>>>>>> Cc: Joseph Qi
>>>>>> Cc: Daniel Vetter
>>>>>> Cc: Arnd Bergmann
>>>>>
>>>>> Acked-by: Tejun Heo
>>>>>
>>>>> Jens, it'd probably be best to route this through block tree.
>>>>
>>>> Oops, this requires an earlier patch to move the for_each_if def to a
>>>> common header and should be routed together.
>>>
>>> Yeah, this is a problem with the submission.
>>>
>>> Always (ALWAYS) CC folks on at least the cover letter and generic
>>> earlier patches. Getting just one patch sent like this is mostly
>>> useless, and causes more harm than good.
>>
>> Ime sending a patch with more than 20 or so recipients means it gets
>> stuck everywhere in moderation queues. Or outright spam filters. I
>> thought the correct way to do this is to cc: mailing lists (lkml has
>> them all), but apparently that's not how it's done. Despite that all
>> the patch series I get never have the cover letter addressed to me
>> either.
>>
>> So what's the magic way to make this possible?
>
> Jens' advice is crap.
>
> There is no generic way to make this possible.
Nobody claimed there was. And the advice is perfectly fine,
sending out patches to folks that have hidden dependencies on other
patches is a no-go.
> BCC's don't work, series that touch multiple subsystems
> get rejected when the recipient list is too large.
>
> I think you did it correctly.
Clearly that's not the case, regardless of what you think.
Thanks for your invaluable and useful feedback, sharing your vast
experience in patchsets with dependencies.
--
Jens Axboe
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 02/12] blk: use for_each_if
On 7/11/18 3:08 PM, Daniel Vetter wrote: > On Wed, Jul 11, 2018 at 10:06 PM, Tejun Heo wrote: >> On Wed, Jul 11, 2018 at 01:31:51PM -0600, Jens Axboe wrote: >>> I don't think there's a git easy way of sending it out outside of >>> just ensuring that everybody is CC'ed on everything. I don't mind >>> that at all. I don't subscribe to lkml, and the patches weren't >>> sent to linux-block. Hence all I see is this stand-alone patch, >>> and logic would dictate that it's stand-alone (but it isn't). > > Hm yeah I forgot to add linux-block. But others where there's no > dedicated list (or get_maintainers.pl didn't have one) also complained > about not getting Cc'ed, and I can't Cc everyone for sweeping changes. I don't personally see a problem with just CC'ing everyone. >> What I sometimes do is including a short blurb on each patch giving >> the overview and action hints (e.g. this is part of patchset doing XYZ >> and should be routed such and such). It's a bit redundant but has >> worked pretty well for patchsets with dependenat & sweeping changes. > > Yeah I guess I can just copypaste/summarize patch 1 to all the > subsequent patches, sounds like the best option. Another approach might be to submit the first independent patch separately. Once that's in the kernel, you can send out the rest as independent patches instead of doing a cross-kernel series that all depend on one single patch. Seems to me that's where you run into issues, and it can be avoided quite easily. -- Jens Axboe ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 02/12] blk: use for_each_if
On 7/11/18 12:50 PM, Daniel Vetter wrote:
> On Wed, Jul 11, 2018 at 8:30 PM, Jens Axboe wrote:
>> On 7/11/18 10:45 AM, Tejun Heo wrote:
>>> On Wed, Jul 11, 2018 at 09:40:58AM -0700, Tejun Heo wrote:
>>>> On Mon, Jul 09, 2018 at 10:36:40AM +0200, Daniel Vetter wrote:
>>>>> Makes the macros resilient against if {} else {} blocks right
>>>>> afterwards.
>>>>>
>>>>> Signed-off-by: Daniel Vetter
>>>>> Cc: Tejun Heo
>>>>> Cc: Jens Axboe
>>>>> Cc: Shaohua Li
>>>>> Cc: Kate Stewart
>>>>> Cc: Greg Kroah-Hartman
>>>>> Cc: Joseph Qi
>>>>> Cc: Daniel Vetter
>>>>> Cc: Arnd Bergmann
>>>>
>>>> Acked-by: Tejun Heo
>>>>
>>>> Jens, it'd probably be best to route this through block tree.
>>>
>>> Oops, this requires an earlier patch to move the for_each_if def to a
>>> common header and should be routed together.
>>
>> Yeah, this is a problem with the submission.
>>
>> Always (ALWAYS) CC folks on at least the cover letter and generic
>> earlier patches. Getting just one patch sent like this is mostly
>> useless, and causes more harm than good.
>
> Ime sending a patch with more than 20 or so recipients means it gets
> stuck everywhere in moderation queues. Or outright spam filters. I
> thought the correct way to do this is to cc: mailing lists (lkml has
> them all), but apparently that's not how it's done. Despite that all
> the patch series I get never have the cover letter addressed to me
> either.
>
> So what's the magic way to make this possible?
I don't think there's a git easy way of sending it out outside of
just ensuring that everybody is CC'ed on everything. I don't mind
that at all. I don't subscribe to lkml, and the patches weren't
sent to linux-block. Hence all I see is this stand-alone patch,
and logic would dictate that it's stand-alone (but it isn't).
--
Jens Axboe
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 02/12] blk: use for_each_if
On 7/11/18 10:45 AM, Tejun Heo wrote:
> On Wed, Jul 11, 2018 at 09:40:58AM -0700, Tejun Heo wrote:
>> On Mon, Jul 09, 2018 at 10:36:40AM +0200, Daniel Vetter wrote:
>>> Makes the macros resilient against if {} else {} blocks right
>>> afterwards.
>>>
>>> Signed-off-by: Daniel Vetter
>>> Cc: Tejun Heo
>>> Cc: Jens Axboe
>>> Cc: Shaohua Li
>>> Cc: Kate Stewart
>>> Cc: Greg Kroah-Hartman
>>> Cc: Joseph Qi
>>> Cc: Daniel Vetter
>>> Cc: Arnd Bergmann
>>
>> Acked-by: Tejun Heo
>>
>> Jens, it'd probably be best to route this through block tree.
>
> Oops, this requires an earlier patch to move the for_each_if def to a
> common header and should be routed together.
Yeah, this is a problem with the submission.
Always (ALWAYS) CC folks on at least the cover letter and generic
earlier patches. Getting just one patch sent like this is mostly
useless, and causes more harm than good.
--
Jens Axboe
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Make vblank evade warnings optional
On 05/08/2017 01:25 AM, Daniel Vetter wrote: > On Sun, May 07, 2017 at 07:52:14PM -0600, Jens Axboe wrote: >> On 05/07/2017 11:56 AM, Daniel Vetter wrote: >>> On Sun, May 7, 2017 at 7:46 PM, Jens Axboe wrote: >>>> On 05/07/2017 11:12 AM, ville.syrj...@linux.intel.com wrote: >>>>> From: Ville Syrjälä >>>>> >>>>> Add a new Kconfig option to enable/disable the extra warnings >>>>> from the vblank evade code. For now we'll keep the warning >>>>> about an actually missed vblank always enabled as that can have >>>>> an actual user visible impact. But if we miss the deadline >>>>> othrwise there's no real need to bother the user with that. >>>>> We'll want these warnings enabled during development however >>>>> so that we can catch regressions. >>>>> >>>>> Based on the reports it looks like this is still very easy >>>>> to hit on SKL, so we have more work ahead of us to optimize >>>>> the crtiical section further. >>>> >>>> Shouldn't it just be a debug printk or something instead, so that normal >>>> people don't see it, but the folks that turn on debugging can get the >>>> info they need? Seems silly to add a kconfig option for this. >>> >>> I guess we could keep it as debug for users, but we want to make this >>> a hard failure on our CI machines. Kconfig knob is the easiest to roll >>> out to all machines. >> >> Wouldn't a module parameter be more useful then, as an opt-in >> to catch these violations. >> >> Nobody is going to know wtf to set this kconfig option to. > > They're all hidden behind an overall i915 debugging option which tells you > not to enable it. You won't see this. OK, that does improve things a bit. -- Jens Axboe ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Make vblank evade warnings optional
On 05/07/2017 11:56 AM, Daniel Vetter wrote: > On Sun, May 7, 2017 at 7:46 PM, Jens Axboe wrote: >> On 05/07/2017 11:12 AM, ville.syrj...@linux.intel.com wrote: >>> From: Ville Syrjälä >>> >>> Add a new Kconfig option to enable/disable the extra warnings >>> from the vblank evade code. For now we'll keep the warning >>> about an actually missed vblank always enabled as that can have >>> an actual user visible impact. But if we miss the deadline >>> othrwise there's no real need to bother the user with that. >>> We'll want these warnings enabled during development however >>> so that we can catch regressions. >>> >>> Based on the reports it looks like this is still very easy >>> to hit on SKL, so we have more work ahead of us to optimize >>> the crtiical section further. >> >> Shouldn't it just be a debug printk or something instead, so that normal >> people don't see it, but the folks that turn on debugging can get the >> info they need? Seems silly to add a kconfig option for this. > > I guess we could keep it as debug for users, but we want to make this > a hard failure on our CI machines. Kconfig knob is the easiest to roll > out to all machines. Wouldn't a module parameter be more useful then, as an opt-in to catch these violations. Nobody is going to know wtf to set this kconfig option to. -- Jens Axboe ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Make vblank evade warnings optional
On 05/07/2017 11:12 AM, ville.syrj...@linux.intel.com wrote: > From: Ville Syrjälä > > Add a new Kconfig option to enable/disable the extra warnings > from the vblank evade code. For now we'll keep the warning > about an actually missed vblank always enabled as that can have > an actual user visible impact. But if we miss the deadline > othrwise there's no real need to bother the user with that. > We'll want these warnings enabled during development however > so that we can catch regressions. > > Based on the reports it looks like this is still very easy > to hit on SKL, so we have more work ahead of us to optimize > the crtiical section further. Shouldn't it just be a debug printk or something instead, so that normal people don't see it, but the folks that turn on debugging can get the info they need? Seems silly to add a kconfig option for this. -- Jens Axboe ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] drm] Atomic update on pipe (A) took 119 us, max time under evasion is 100 us
On 05/04/2017 11:42 AM, Ville Syrjälä wrote: > On Thu, May 04, 2017 at 09:26:09AM -0600, Jens Axboe wrote: >> Hi, >> >> Running current -git on my laptop (20FB, X1 Carbon gen4, skylake), I get >> a lot of the below warnings. Things seem to work fine (in fact it seems >> faster in general use than previously), but it's a lot of warning spew. >> >> [ 764.877978] [drm] Atomic update on pipe (A) took 156 us, max time under >> evasion is 100 us > > I tried to optimize this a bit recently but indeed it's stil known to be too > slow. Looks like all of that stuff did land in Linus's tree already, > so presumably you have it all already. Yes, this is Linus' tree... > I did have some further ideas that should help but I got sidetracked by > other things before I managed to finish the work. I guess I'll need to get > back on that horse and try to finish what I started. > > In the meantime, maybe we should just silence this error spew again > until we're more confident about meeting the deadlines. Maarten? > > Do you have lockdep enabled BTW? Based on what I've seen lockdep does > seem be a major contributor to slowness here. Nope, running a fairly optimized build on my laptop. -- Jens Axboe ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] drm] Atomic update on pipe (A) took 119 us, max time under evasion is 100 us
under evasion is 100 us [ 3022.487159] [drm] Atomic update on pipe (A) took 178 us, max time under evasion is 100 us [ 3035.134699] [drm] Atomic update on pipe (A) took 109 us, max time under evasion is 100 us [ 3066.834414] [drm] Atomic update on pipe (A) took 178 us, max time under evasion is 100 us [ 3089.166099] [drm] Atomic update on pipe (A) took 182 us, max time under evasion is 100 us [ 3094.565358] [drm] Atomic update on pipe (A) took 178 us, max time under evasion is 100 us [ 3099.498272] [drm] Atomic update on pipe (A) took 184 us, max time under evasion is 100 us [ 3114.162375] [drm] Atomic update on pipe (A) took 155 us, max time under evasion is 100 us [ 3344.500953] [drm] Atomic update on pipe (A) took 112 us, max time under evasion is 100 us [ 3374.664435] [drm] Atomic update on pipe (A) took 175 us, max time under evasion is 100 us [ 3398.468489] [drm] Atomic update on pipe (A) took 168 us, max time under evasion is 100 us [ 3398.864789] [drm] Atomic update on pipe (A) took 158 us, max time under evasion is 100 us [ 3399.733254] [drm] Atomic update on pipe (A) took 168 us, max time under evasion is 100 us [ 3416.316141] [drm] Atomic update on pipe (A) took 175 us, max time under evasion is 100 us [ 3424.065187] [drm] Atomic update on pipe (A) took 149 us, max time under evasion is 100 us [ 3942.736123] [drm] Atomic update on pipe (A) took 255 us, max time under evasion is 100 us [ 4366.622324] [drm] Atomic update on pipe (A) took 274 us, max time under evasion is 100 us [ 5401.716629] [drm] Atomic update on pipe (A) took 198 us, max time under evasion is 100 us [ 5587.292655] [drm] Atomic update on pipe (A) took 175 us, max time under evasion is 100 us [ 7272.084592] [drm] Atomic update on pipe (A) took 233 us, max time under evasion is 100 us [ 7420.257422] [drm] Atomic update on pipe (A) took 213 us, max time under evasion is 100 us [ 7938.997404] [drm] Atomic update on pipe (A) took 241 us, max time under evasion is 100 us [ 8341.927989] [drm] Atomic update on pipe (A) took 119 us, max time under evasion is 100 us [ 8549.094800] [drm] Atomic update on pipe (A) took 221 us, max time under evasion is 100 us [ 8550.202973] [drm] Atomic update on pipe (A) took 296 us, max time under evasion is 100 us [ 8648.787792] [drm] Atomic update on pipe (A) took 214 us, max time under evasion is 100 us -- Jens Axboe ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] Limit busywaiting
On 11/18/2015 02:56 AM, Chris Wilson wrote: This should filter out all explicit wait requests from userspace and only apply busywaiting to circumstances where we are forced to drain the GPU of old requests. With the 2 microsecond timeout from before, this still seems to preserve the speed up in stress tests and cancel the busywaiting for desktop loads. Chris, I tested these 3 on top of the previous 2us limit patch, and it seems to work fine. You can add: Tested-by: Jens Axboe to them, if you'd like. -- Jens Axboe ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Limit the busy wait on requests to 2us not 10ms!
On 11/15/2015 06:32 AM, Chris Wilson wrote: When waiting for high frequency requests, the finite amount of time required to set up the irq and wait upon it limits the response rate. By busywaiting on the request completion for a short while we can service the high frequency waits as quick as possible. However, if it is a slow request, we want to sleep as quickly as possible. The tradeoff between waiting and sleeping is roughly the time it takes to sleep on a request, on the order of a microsecond. Based on measurements from big core, I have set the limit for busywaiting as 2 microseconds. The code currently uses the jiffie clock, but that is far too coarse (on the order of 10 milliseconds) and results in poor interactivity as the CPU ends up being hogged by slow requests. To get microsecond resolution we need to use a high resolution timer. The cheapest of which is polling local_clock(), but that is only valid on the same CPU. If we switch CPUs because the task was preempted, we can also use that as an indicator that the system is too busy to waste cycles on spinning and we should sleep instead. I tried this (1+2), and it feels better. However, I added some counters just to track how well it's faring: [ 491.077612] i915: invoked=7168, success=50 so out of 6144 invocations, we only avoided going to sleep 49 of those times. As a percentage, that's 99.3% of the time we spun 2usec for no good reason other than to burn up more of my battery. So the reason there's an improvement for me is that we're just not spinning the 10ms anymore, however we're still just wasting time for my use case. I'd recommend putting this behind some option so that people can enable it and play with it if they want, but not making it default to on until some more clever tracking has been added to dynamically adapt to on when to poll and when not to. It should not be a default-on type of thing until it's closer to doing the right thing for a normal workload, not just some synthetic benchmark. -- Jens Axboe ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm: Fix locking for sysfs dpms file
On 09/29/2015 01:56 AM, Daniel Vetter wrote: With atomic drivers we need to make sure that (at least in general) property reads hold the right locks. But the legacy dpms property is special and can be read locklessly. Since userspace loves to just randomly look at that all the time (like with "status") do that. To make it clear that we play tricks use the READ_ONCE compiler barrier (and also for paranoia). Note that there's not really anything bad going on since even with the new atomic paths we eventually end up not chasing any pointers (and hence possibly freed memory and other fun stuff). The locking WARNING has been added in commit 88a48e297b3a3bac6022c03babfb038f1a886cea Author: Rob Clark Date: Thu Dec 18 16:01:50 2014 -0500 drm: add atomic properties but since drivers are converting not everyone will have seen this from the start. Jens reported this and submitted a patch to just grab the mode_config.connection_mutex, but we can do a bit better. v2: Remove unused variables I failed to git add for real. Reported-by: Jens Axboe Cc: Jens Axboe Cc: Rob Clark Cc: sta...@vger.kernel.org Signed-off-by: Daniel Vetter Works for me, thanks Daniel. Tested-by: Jens Axboe -- Jens Axboe ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
