[issue35517] selector.EpollSelector: add new parameter to support EPOLLEXCLUSIVE

[Manjusaka]

Manjusaka  added the comment:

ping

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue35517] selector.EpollSelector: add new parameter to support EPOLLEXCLUSIVE

[Manjusaka]

Manjusaka  added the comment:

> `selectors` makes underlying implementations irrelavant to most users since 
> we can simply use `DefaultSelector`

I agree with this. 

> If you know you want to add EPOLL_EXCLUSIVE, why not just use `select.epoll`?

I don't think so. 

If I use the `select` directly, that means I have to give up all the features 
what `selectors` support such as binding a `data` to a fd, that also means I 
have to do some repeat work myself. I don't think it's a good idea.

So, why not change this API when we can keep the compatibility?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue35517] selector.EpollSelector: add new parameter to support EPOLLEXCLUSIVE

[Xiang Zhang]

Xiang Zhang  added the comment:

> I don't think to make selector be a public property is a good idea. It will 
> break the whole system integrity.

If exposing a private property is not a good idea, another choice may be 
construct a selector with a customized I/O multiplexer, adding an optional 
parameter to the __init__.

But actually I'm -1 to this change. `selectors` makes underlying 
implementations irrelavant to most users since we can simply use 
`DefaultSelector`(maybe why only read/write events are valid now?). But you are 
seeking to add implementation specific details back. If you know you want to 
add EPOLL_EXCLUSIVE, why not just use `select.epoll`? A single selector doesn't 
do much more than the underlying multiplexer.

--
nosy: +xiang.zhang

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue35517] selector.EpollSelector: add new parameter to support EPOLLEXCLUSIVE

[Manjusaka]

Change by Manjusaka :


--
nosy:  -asvetlov

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue35517] selector.EpollSelector: add new parameter to support EPOLLEXCLUSIVE

[Andrew Svetlov]

Andrew Svetlov  added the comment:

Hmm, I forgot kqueue usage details: it operated not only with int flags but 
more complex structures.
Giampaolo, thanks for pointing on!

Please let me sleep on it.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue35517] selector.EpollSelector: add new parameter to support EPOLLEXCLUSIVE

[Guido van Rossum]

Change by Guido van Rossum :


--
nosy:  -gvanrossum

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue35517] selector.EpollSelector: add new parameter to support EPOLLEXCLUSIVE

[Manjusaka]

Manjusaka  added the comment:

Actually, in my implementation, it also supports POLL with the different event.

I don't think to make selector be a public property is a good idea. It will 
break the whole system integrity.

Please think about it, if people want to use epoll/poll with some special 
event, they must use it like this.

>  s = selectors.EpollSelector()
>  s.selector.register(fd,select.EPOLLIN | select.EPOLLEXCLUSIVE)
>  s.selector.modify(fd,select.EPOLLOU | select.EPOLLEXCLUSIVE)

Here's a question, why we support the register?

I think it will make people don't care about the detail.

So, as you say, it's a little bit complicated, but it will help people don't 
care about the selector property detail, they just use the argument when they 
want to use it.

I think it's worth it.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue35517] selector.EpollSelector: add new parameter to support EPOLLEXCLUSIVE

[Giampaolo Rodola']

Giampaolo Rodola'  added the comment:

I took a look at your PR. As the PR currently stands it only works with epoll() 
selector. For the other selectors this is just an extra argument which does 
nothing, so it complicates the API of 2 methods for no real gain. Also, a 
single argument is not compatible with kqueue() because kqueue() requires two 
distinct `KQ_FILTER_*` and `KQ_EV_*` constants which cannot be ORed together. 

Instead, I think simply turning the underlying selector instance into a public 
property is more flexible and less complex. On Linux you'll do:

>>> s = selectors.EpollSelector()
>>> s.register(fd, EVENT_READ)
>>> s.selector.modify(fd, select.EPOLLEXCLUSIVE)
```

...and on BSD:

>>> s = selectors.KqueueSelector()
>>> s.register(fd, EVENT_READ)
>>> k = s.selector.kevent(fd, select.KQ_FILTER_READ, select.KQ_EV_CLEAR)
>>> s.selector.control([k], 0, 0)
```

Alternatively we could just keep `DefaultSelector._selector` private and 
document it. Personally I'd also be OK with that even though I'm not sure if 
there if there are precedents of documenting private APIs.

--
nosy: +asvetlov, gvanrossum

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue35517] selector.EpollSelector: add new parameter to support EPOLLEXCLUSIVE

[STINNER Victor]

STINNER Victor  added the comment:

I mean that using extra_events to get access to EPOLLEXCLUSIVE, there is no 
need later to add a new parameter for select.EPOLLONESHOT: it would be "future 
proof"!

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue35517] selector.EpollSelector: add new parameter to support EPOLLEXCLUSIVE

[Manjusaka]

Manjusaka  added the comment:

Vicor:

> Moreover, we directly support any EPOLL constant exposed in the select 
> module. No need to change the API.

I don't think so


In class _PollLikeSelector ,here's register method

def register(self, fileobj, events, data=None):
key = super().register(fileobj, events, data)
poller_events = 0
if events & EVENT_READ:
poller_events |= self._EVENT_READ
if events & EVENT_WRITE:
poller_events |= self._EVENT_WRITE

this code filter the event that people push in it. It only supports 
select.EPOLLIN and select.EPOLLOUT

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue35517] selector.EpollSelector: add new parameter to support EPOLLEXCLUSIVE

[Manjusaka]

Manjusaka  added the comment:

OK, I will change my code

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue35517] selector.EpollSelector: add new parameter to support EPOLLEXCLUSIVE

[STINNER Victor]

STINNER Victor  added the comment:

I prefer Giampaolo since discussed flags are very specific to epoll(): select() 
doesn't support them for example, nor kqueue nor devpoll (not *yet*).

If we add a keyword-parameter, to me, it sounds like it's something "portable" 
working on multiple platforms and then you need hasattr():

if exclusive and hasattr(select, "EPOLLEXCLUSIVE"):
epoll_events |= select.EPOLLEXCLUSIVE

If the caller pass select.EPOLLEXCLUSIVE, hasattr() is useless.

Moreover, we directly support any EPOLL constant exposed in the select module. 
No need to change the API.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue35517] selector.EpollSelector: add new parameter to support EPOLLEXCLUSIVE

[Manjusaka]

Manjusaka  added the comment:

In my opinion

selectors is an abstract for select, so I don't think allow people use select.* 
in selector is a good idea.

like this

> s.register(fd, EVENT_READ, extra_events=select.EPOLLEXCLUSIVE | 
> select.EPOLLONESHOT)

Because the multiple epoll's params are supported in different Python version 
and Linux Kernel version.

So, I think it's a good idea to support different epoll's params by 
keyword-only param in register method.

It's also convenient to check the params

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue35517] selector.EpollSelector: add new parameter to support EPOLLEXCLUSIVE

[Giampaolo Rodola']

Giampaolo Rodola'  added the comment:

I see. Then I would say it's a matter of deciding what's the best API to 
provide. Another possibility is to promote the underlying epoll() instance as a 
public property, so one can do:

>>> s = selectors.EpollSelector()
>>> s.register(fd, EVENT_READ)
>>> s.selector.modify(fd, select.EPOLLEXCLUSIVE)

That raises the question whether all selector classes should have a public 
"selector" attribute. poll() and devpoll() related classes may need it for 
POLLPRI, POLLRDHUP, POLLWRBAND or others (whatever their use case is). kqueue() 
also has it's own specific constants (KQ_FILTER_* and KQ_EV_*). The only one 
where a public "selector" property would be useless is SelectSelector.

--
stage: patch review -> 

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue35517] selector.EpollSelector: add new parameter to support EPOLLEXCLUSIVE

[Manjusaka]

Change by Manjusaka :


--
keywords: +patch
pull_requests: +10433
stage:  -> patch review

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue35517] selector.EpollSelector: add new parameter to support EPOLLEXCLUSIVE

[Manjusaka]

Manjusaka  added the comment:

Hello rodola

Here's detail:

In the server, we use multiprocess to handle the request with share the same 
FD. 

OK, when a request comes, all the process wake up and try to accept the request 
but only one process will accept success and the others will raise an Exception 
if we use the epoll without EPOLLEXCLUSIVE.

That means there will waste the performance. This effect is named thundering 
herd.

But if we use epoll with EPOLLEXCLUSIVE, when the envents happen, only one 
process will wake and try to handle the event.

So, I think it's necessary to support the EPOLLEXCLUSIVE in 
selectors.EpollSelector

Actually, Python support EPOLLEXCLUSIVE official since 3.7. It's time to 
support it in selectors

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue35517] selector.EpollSelector: add new parameter to support EPOLLEXCLUSIVE

[Giampaolo Rodola']

Giampaolo Rodola'  added the comment:

I'm not sure I understand what EPOLLEXCLUSIVE is about. Could you provide a use 
case?

Also, there are other constants which may also be useful such as EPOLLWAKEUP 
and EPOLLONESHOT:
http://man7.org/linux/man-pages/man2/epoll_ctl.2.html
So perhaps it makes more sense to expose a lower-level "extra_events" argument 
instead, which is more flexible and also saves us from the hassle of describing 
what the new argument is about (which really is a Linux kernel thing) in the 
doc:

>>> s.register(fd, EVENT_READ, extra_events=select.EPOLLEXCLUSIVE | 
>>> select.EPOLLONESHOT)

That raises the question whether we should also have an "extra_events" argument 
for modify() method (probably yes). 

But again, I think it makes sense to understand what's the use case of these 
constants first, because if they are rarely used maybe who needs them can 
simply look at the source and use s._selector.modify() directly.

--
nosy: +neologix, yselivanov

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue35517] selector.EpollSelector: add new parameter to support EPOLLEXCLUSIVE

[Manjusaka]

Manjusaka  added the comment:

"a keyword-only parameter" is good I'll take it done

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue35517] selector.EpollSelector: add new parameter to support EPOLLEXCLUSIVE

[Manjusaka]

Manjusaka  added the comment:

I will work on a PR

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue35517] selector.EpollSelector: add new parameter to support EPOLLEXCLUSIVE

[STINNER Victor]

STINNER Victor  added the comment:

if exclusive and hasattr(select, "EPOLLEXCLUSIVE"):
epoll_events |= select.EPOLLEXCLUSIVE

Maybe NotImplementedError would be more appropriate rather than silently ignore 
the error?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue35517] selector.EpollSelector: add new parameter to support EPOLLEXCLUSIVE

[STINNER Victor]

STINNER Victor  added the comment:

EPOLLEXCLUSIVE doc from Linux manual page:

"""
EPOLLEXCLUSIVE (since Linux 4.5)

Sets an exclusive wakeup mode for the epoll file descriptor that
is  being  attached  to  the target file descriptor, fd.  When a
wakeup event occurs and  multiple  epoll  file  descriptors  are
attached  to  the  same target file using EPOLLEXCLUSIVE, one or
more of the epoll file descriptors will receive  an  event  with
epoll_wait(2).   The  default in this scenario (when EPOLLEXCLU‐
SIVE is not set) is for all epoll file descriptors to receive an
event.   EPOLLEXCLUSIVE  is  thus useful for avoiding thundering
herd problems in certain scenarios.

If the same file descriptor is in multiple epoll instances, some
with  the  EPOLLEXCLUSIVE  flag, and others without, then events
will be provided to all epoll instances  that  did  not  specify
EPOLLEXCLUSIVE, and at least one of the epoll instances that did
specify EPOLLEXCLUSIVE.

The following  values  may  be  specified  in  conjunction  with
EPOLLEXCLUSIVE:  EPOLLIN,  EPOLLOUT,  EPOLLWAKEUP,  and EPOLLET.
EPOLLHUP and EPOLLERR can also be specified,  but  this  is  not
required:  as  usual,  these  events are always reported if they
occur, regardless of  whether  they  are  specified  in  events.
Attempts  to  specify  other  values  in  events yield an error.
EPOLLEXCLUSIVE may be used only in an  EPOLL_CTL_ADD  operation;
attempts  to  employ  it  with EPOLL_CTL_MOD yield an error.  If
EPOLLEXCLUSIVE has been set using epoll_ctl(), then a subsequent
EPOLL_CTL_MOD on the same epfd, fd pair yields an error.  A call
to epoll_ctl() that specifies EPOLLEXCLUSIVE in events and spec‐
ifies  the  target  file descriptor fd as an epoll instance will
likewise fail.  The error in all of these cases is EINVAL.
"""

See also https://lwn.net/Articles/633422/

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue35517] selector.EpollSelector: add new parameter to support EPOLLEXCLUSIVE

[STINNER Victor]

STINNER Victor  added the comment:

> Add a keyword argument for selector.EpollSelector with default value.

I suggest to use a keyword-only parameter:

def register(self, fileobj, events, data=None, *, exclusive=False):

Do you want to work on a pull request?

--
nosy: +giampaolo.rodola, vstinner
title: enhhance for selector.EpollSelector -> selector.EpollSelector: add new 
parameter to support EPOLLEXCLUSIVE
versions:  -Python 3.7

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com