Re: [RFC][PATCH v2] epoll: allow EPOLLWAKEUP flag if PM_SLEEP is enabled
On Thursday, November 14, 2013 09:59:44 PM Amit Pundir wrote: > On 14 November 2013 03:14, Rafael J. Wysocki wrote: > > On Wednesday, November 13, 2013 01:35:38 PM Amit Pundir wrote: > >> On 13 November 2013 05:29, Rafael J. Wysocki wrote: > >> > On Wednesday, November 13, 2013 02:22:28 AM Amit Pundir wrote: > >> >> ep_create_wakeup_source() reports ENOMEM > >> > > >> > That needs to be fixed too. I suppose we can make the > >> > wakeup_source_register() > >> > stub for CONFIG_PM_SLEEP unset return ERR_PTR(-ENOSYS) or something like > >> > that > >> > and ep_create_wakeup_source() return that instead of -ENOMEM. It looks > >> > like > >> > eventpoll.c is the only user of it built for CONFIG_PM_SLEEP unset, but > >> > that > >> > needs to be double checked. > >> > >> Instead of modifying wakeup_source_register() stub, what if I make > >> ep_create_wakeup_source() static inline as well and use its stub to > >> return -ENOSYS when CONFIG_PM_SLEEP is not set? > >> ep_create_wakeup_source() is used only in fs/eventpoll.c anyway. > > > > Well, you can do that too. > > > > On second thought we may skip modifying ep_create_wakeup_source() or > wakeup_source_register() altogether because once we drop EPOLLWAKEUP > from epoll events mask(if PM_SLEEP is unset) then I don't see us > running into ep_create_wakeup_source() again. And the only reason for > ep_create_wakeup_source() failure will be -ENOMEM as far as I can see. OK > >> >> if wakeup_source_register() > >> >> returns NULL. ep_create_wakeup_source() assumes that NULL is only > >> >> returned if we run into ENOMEM but NULL is also returned when > >> >> CONFIG_PM_SLEEP is disabled. > >> >> > >> >> Signed-off-by: Amit Pundir > >> >> --- > >> >> Changed in v2: > >> >> Using static inline functions instead of #ifdefs > >> >> --- > >> >> fs/eventpoll.c |3 +-- > >> >> include/uapi/linux/eventpoll.h | 12 > >> >> 2 files changed, 13 insertions(+), 2 deletions(-) > >> >> > >> >> diff --git a/fs/eventpoll.c b/fs/eventpoll.c > >> >> index 473e09d..10f9c43 100644 > >> >> --- a/fs/eventpoll.c > >> >> +++ b/fs/eventpoll.c > >> >> @@ -1820,8 +1820,7 @@ SYSCALL_DEFINE4(epoll_ctl, int, epfd, int, op, > >> >> int, fd, > >> >> goto error_tgt_fput; > >> >> > >> >> /* Check if EPOLLWAKEUP is allowed */ > >> >> - if ((epds.events & EPOLLWAKEUP) && !capable(CAP_BLOCK_SUSPEND)) > >> >> - epds.events &= ~EPOLLWAKEUP; > >> >> + ep_epollwakeup_check(&epds.events); > >> > > >> > The "check" part of the name kind of suggests that the function will not > >> > change > >> > things. What about ep_adjust_epollwakeup() or something along these > >> > lines? > >> > >> I see couple of ep_set_* functions in eventpoll.c. Does it make sense > >> to have something like ep_set_epollwakeup()? > > > > This particular one doesn't really set anything. I suppose that a name like > > "ep_take_care_of_epollwakeup" might be somewhat closer to what it really > > does ... > > I'm running out of ideas on this one, lets go with > "ep_take_care_of_epollwakeup". Well, that's fine by me, if no one else has any better ideas. Thanks! -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC][PATCH v2] epoll: allow EPOLLWAKEUP flag if PM_SLEEP is enabled
On 14 November 2013 03:14, Rafael J. Wysocki wrote: > On Wednesday, November 13, 2013 01:35:38 PM Amit Pundir wrote: >> On 13 November 2013 05:29, Rafael J. Wysocki wrote: >> > On Wednesday, November 13, 2013 02:22:28 AM Amit Pundir wrote: >> >> ep_create_wakeup_source() reports ENOMEM >> > >> > That needs to be fixed too. I suppose we can make the >> > wakeup_source_register() >> > stub for CONFIG_PM_SLEEP unset return ERR_PTR(-ENOSYS) or something like >> > that >> > and ep_create_wakeup_source() return that instead of -ENOMEM. It looks >> > like >> > eventpoll.c is the only user of it built for CONFIG_PM_SLEEP unset, but >> > that >> > needs to be double checked. >> >> Instead of modifying wakeup_source_register() stub, what if I make >> ep_create_wakeup_source() static inline as well and use its stub to >> return -ENOSYS when CONFIG_PM_SLEEP is not set? >> ep_create_wakeup_source() is used only in fs/eventpoll.c anyway. > > Well, you can do that too. > On second thought we may skip modifying ep_create_wakeup_source() or wakeup_source_register() altogether because once we drop EPOLLWAKEUP from epoll events mask(if PM_SLEEP is unset) then I don't see us running into ep_create_wakeup_source() again. And the only reason for ep_create_wakeup_source() failure will be -ENOMEM as far as I can see. >> >> if wakeup_source_register() >> >> returns NULL. ep_create_wakeup_source() assumes that NULL is only >> >> returned if we run into ENOMEM but NULL is also returned when >> >> CONFIG_PM_SLEEP is disabled. >> >> >> >> Signed-off-by: Amit Pundir >> >> --- >> >> Changed in v2: >> >> Using static inline functions instead of #ifdefs >> >> --- >> >> fs/eventpoll.c |3 +-- >> >> include/uapi/linux/eventpoll.h | 12 >> >> 2 files changed, 13 insertions(+), 2 deletions(-) >> >> >> >> diff --git a/fs/eventpoll.c b/fs/eventpoll.c >> >> index 473e09d..10f9c43 100644 >> >> --- a/fs/eventpoll.c >> >> +++ b/fs/eventpoll.c >> >> @@ -1820,8 +1820,7 @@ SYSCALL_DEFINE4(epoll_ctl, int, epfd, int, op, int, >> >> fd, >> >> goto error_tgt_fput; >> >> >> >> /* Check if EPOLLWAKEUP is allowed */ >> >> - if ((epds.events & EPOLLWAKEUP) && !capable(CAP_BLOCK_SUSPEND)) >> >> - epds.events &= ~EPOLLWAKEUP; >> >> + ep_epollwakeup_check(&epds.events); >> > >> > The "check" part of the name kind of suggests that the function will not >> > change >> > things. What about ep_adjust_epollwakeup() or something along these lines? >> >> I see couple of ep_set_* functions in eventpoll.c. Does it make sense >> to have something like ep_set_epollwakeup()? > > This particular one doesn't really set anything. I suppose that a name like > "ep_take_care_of_epollwakeup" might be somewhat closer to what it really does > ... I'm running out of ideas on this one, lets go with "ep_take_care_of_epollwakeup". Regards, Amit Pundir > > Thanks! > > -- > I speak only for myself. > Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC][PATCH v2] epoll: allow EPOLLWAKEUP flag if PM_SLEEP is enabled
On Wednesday, November 13, 2013 01:35:38 PM Amit Pundir wrote: > On 13 November 2013 05:29, Rafael J. Wysocki wrote: > > On Wednesday, November 13, 2013 02:22:28 AM Amit Pundir wrote: > >> ep_create_wakeup_source() reports ENOMEM > > > > That needs to be fixed too. I suppose we can make the > > wakeup_source_register() > > stub for CONFIG_PM_SLEEP unset return ERR_PTR(-ENOSYS) or something like > > that > > and ep_create_wakeup_source() return that instead of -ENOMEM. It looks like > > eventpoll.c is the only user of it built for CONFIG_PM_SLEEP unset, but that > > needs to be double checked. > > Instead of modifying wakeup_source_register() stub, what if I make > ep_create_wakeup_source() static inline as well and use its stub to > return -ENOSYS when CONFIG_PM_SLEEP is not set? > ep_create_wakeup_source() is used only in fs/eventpoll.c anyway. Well, you can do that too. > >> if wakeup_source_register() > >> returns NULL. ep_create_wakeup_source() assumes that NULL is only > >> returned if we run into ENOMEM but NULL is also returned when > >> CONFIG_PM_SLEEP is disabled. > >> > >> Signed-off-by: Amit Pundir > >> --- > >> Changed in v2: > >> Using static inline functions instead of #ifdefs > >> --- > >> fs/eventpoll.c |3 +-- > >> include/uapi/linux/eventpoll.h | 12 > >> 2 files changed, 13 insertions(+), 2 deletions(-) > >> > >> diff --git a/fs/eventpoll.c b/fs/eventpoll.c > >> index 473e09d..10f9c43 100644 > >> --- a/fs/eventpoll.c > >> +++ b/fs/eventpoll.c > >> @@ -1820,8 +1820,7 @@ SYSCALL_DEFINE4(epoll_ctl, int, epfd, int, op, int, > >> fd, > >> goto error_tgt_fput; > >> > >> /* Check if EPOLLWAKEUP is allowed */ > >> - if ((epds.events & EPOLLWAKEUP) && !capable(CAP_BLOCK_SUSPEND)) > >> - epds.events &= ~EPOLLWAKEUP; > >> + ep_epollwakeup_check(&epds.events); > > > > The "check" part of the name kind of suggests that the function will not > > change > > things. What about ep_adjust_epollwakeup() or something along these lines? > > I see couple of ep_set_* functions in eventpoll.c. Does it make sense > to have something like ep_set_epollwakeup()? This particular one doesn't really set anything. I suppose that a name like "ep_take_care_of_epollwakeup" might be somewhat closer to what it really does ... Thanks! -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC][PATCH v2] epoll: allow EPOLLWAKEUP flag if PM_SLEEP is enabled
On 13 November 2013 05:29, Rafael J. Wysocki wrote: > On Wednesday, November 13, 2013 02:22:28 AM Amit Pundir wrote: >> ep_create_wakeup_source() reports ENOMEM > > That needs to be fixed too. I suppose we can make the > wakeup_source_register() > stub for CONFIG_PM_SLEEP unset return ERR_PTR(-ENOSYS) or something like that > and ep_create_wakeup_source() return that instead of -ENOMEM. It looks like > eventpoll.c is the only user of it built for CONFIG_PM_SLEEP unset, but that > needs to be double checked. Instead of modifying wakeup_source_register() stub, what if I make ep_create_wakeup_source() static inline as well and use its stub to return -ENOSYS when CONFIG_PM_SLEEP is not set? ep_create_wakeup_source() is used only in fs/eventpoll.c anyway. > >> if wakeup_source_register() >> returns NULL. ep_create_wakeup_source() assumes that NULL is only >> returned if we run into ENOMEM but NULL is also returned when >> CONFIG_PM_SLEEP is disabled. >> >> Signed-off-by: Amit Pundir >> --- >> Changed in v2: >> Using static inline functions instead of #ifdefs >> --- >> fs/eventpoll.c |3 +-- >> include/uapi/linux/eventpoll.h | 12 >> 2 files changed, 13 insertions(+), 2 deletions(-) >> >> diff --git a/fs/eventpoll.c b/fs/eventpoll.c >> index 473e09d..10f9c43 100644 >> --- a/fs/eventpoll.c >> +++ b/fs/eventpoll.c >> @@ -1820,8 +1820,7 @@ SYSCALL_DEFINE4(epoll_ctl, int, epfd, int, op, int, fd, >> goto error_tgt_fput; >> >> /* Check if EPOLLWAKEUP is allowed */ >> - if ((epds.events & EPOLLWAKEUP) && !capable(CAP_BLOCK_SUSPEND)) >> - epds.events &= ~EPOLLWAKEUP; >> + ep_epollwakeup_check(&epds.events); > > The "check" part of the name kind of suggests that the function will not > change > things. What about ep_adjust_epollwakeup() or something along these lines? I see couple of ep_set_* functions in eventpoll.c. Does it make sense to have something like ep_set_epollwakeup()? > > And why don't you pass a pointer to epds to it? Wouldn't it be cleaner this > way? Ok. Regards, Amit Pundir > >> >> /* >>* We have to check that the file structure underneath the file >> descriptor >> diff --git a/include/uapi/linux/eventpoll.h b/include/uapi/linux/eventpoll.h >> index 2c267bc..1d139c2 100644 >> --- a/include/uapi/linux/eventpoll.h >> +++ b/include/uapi/linux/eventpoll.h >> @@ -62,4 +62,16 @@ struct epoll_event { >> } EPOLL_PACKED; >> >> >> +#ifdef CONFIG_PM_SLEEP >> +static inline void ep_epollwakeup_check(__u32 *epev) >> +{ >> + if ((*epev & EPOLLWAKEUP) && !capable(CAP_BLOCK_SUSPEND)) >> + *epev &= ~EPOLLWAKEUP; >> +} >> +#else >> +static inline void ep_epollwakeup_check(__u32 *epev) >> +{ >> + *epev &= ~EPOLLWAKEUP; >> +} >> +#endif >> #endif /* _UAPI_LINUX_EVENTPOLL_H */ > > Thanks! > > -- > I speak only for myself. > Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC][PATCH v2] epoll: allow EPOLLWAKEUP flag if PM_SLEEP is enabled
On Wednesday, November 13, 2013 02:22:28 AM Amit Pundir wrote: > ep_create_wakeup_source() reports ENOMEM That needs to be fixed too. I suppose we can make the wakeup_source_register() stub for CONFIG_PM_SLEEP unset return ERR_PTR(-ENOSYS) or something like that and ep_create_wakeup_source() return that instead of -ENOMEM. It looks like eventpoll.c is the only user of it built for CONFIG_PM_SLEEP unset, but that needs to be double checked. > if wakeup_source_register() > returns NULL. ep_create_wakeup_source() assumes that NULL is only > returned if we run into ENOMEM but NULL is also returned when > CONFIG_PM_SLEEP is disabled. > > Signed-off-by: Amit Pundir > --- > Changed in v2: > Using static inline functions instead of #ifdefs > --- > fs/eventpoll.c |3 +-- > include/uapi/linux/eventpoll.h | 12 > 2 files changed, 13 insertions(+), 2 deletions(-) > > diff --git a/fs/eventpoll.c b/fs/eventpoll.c > index 473e09d..10f9c43 100644 > --- a/fs/eventpoll.c > +++ b/fs/eventpoll.c > @@ -1820,8 +1820,7 @@ SYSCALL_DEFINE4(epoll_ctl, int, epfd, int, op, int, fd, > goto error_tgt_fput; > > /* Check if EPOLLWAKEUP is allowed */ > - if ((epds.events & EPOLLWAKEUP) && !capable(CAP_BLOCK_SUSPEND)) > - epds.events &= ~EPOLLWAKEUP; > + ep_epollwakeup_check(&epds.events); The "check" part of the name kind of suggests that the function will not change things. What about ep_adjust_epollwakeup() or something along these lines? And why don't you pass a pointer to epds to it? Wouldn't it be cleaner this way? > > /* >* We have to check that the file structure underneath the file > descriptor > diff --git a/include/uapi/linux/eventpoll.h b/include/uapi/linux/eventpoll.h > index 2c267bc..1d139c2 100644 > --- a/include/uapi/linux/eventpoll.h > +++ b/include/uapi/linux/eventpoll.h > @@ -62,4 +62,16 @@ struct epoll_event { > } EPOLL_PACKED; > > > +#ifdef CONFIG_PM_SLEEP > +static inline void ep_epollwakeup_check(__u32 *epev) > +{ > + if ((*epev & EPOLLWAKEUP) && !capable(CAP_BLOCK_SUSPEND)) > + *epev &= ~EPOLLWAKEUP; > +} > +#else > +static inline void ep_epollwakeup_check(__u32 *epev) > +{ > + *epev &= ~EPOLLWAKEUP; > +} > +#endif > #endif /* _UAPI_LINUX_EVENTPOLL_H */ Thanks! -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[RFC][PATCH v2] epoll: allow EPOLLWAKEUP flag if PM_SLEEP is enabled
ep_create_wakeup_source() reports ENOMEM if wakeup_source_register() returns NULL. ep_create_wakeup_source() assumes that NULL is only returned if we run into ENOMEM but NULL is also returned when CONFIG_PM_SLEEP is disabled. Signed-off-by: Amit Pundir --- Changed in v2: Using static inline functions instead of #ifdefs --- fs/eventpoll.c |3 +-- include/uapi/linux/eventpoll.h | 12 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/fs/eventpoll.c b/fs/eventpoll.c index 473e09d..10f9c43 100644 --- a/fs/eventpoll.c +++ b/fs/eventpoll.c @@ -1820,8 +1820,7 @@ SYSCALL_DEFINE4(epoll_ctl, int, epfd, int, op, int, fd, goto error_tgt_fput; /* Check if EPOLLWAKEUP is allowed */ - if ((epds.events & EPOLLWAKEUP) && !capable(CAP_BLOCK_SUSPEND)) - epds.events &= ~EPOLLWAKEUP; + ep_epollwakeup_check(&epds.events); /* * We have to check that the file structure underneath the file descriptor diff --git a/include/uapi/linux/eventpoll.h b/include/uapi/linux/eventpoll.h index 2c267bc..1d139c2 100644 --- a/include/uapi/linux/eventpoll.h +++ b/include/uapi/linux/eventpoll.h @@ -62,4 +62,16 @@ struct epoll_event { } EPOLL_PACKED; +#ifdef CONFIG_PM_SLEEP +static inline void ep_epollwakeup_check(__u32 *epev) +{ + if ((*epev & EPOLLWAKEUP) && !capable(CAP_BLOCK_SUSPEND)) + *epev &= ~EPOLLWAKEUP; +} +#else +static inline void ep_epollwakeup_check(__u32 *epev) +{ + *epev &= ~EPOLLWAKEUP; +} +#endif #endif /* _UAPI_LINUX_EVENTPOLL_H */ -- 1.7.10.4 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/